diff options
163 files changed, 1990 insertions, 1784 deletions
@@ -469,6 +469,7 @@ group("gn_all") { } if (use_x11) { + deps += [ "//media:player_x11" ] if (target_cpu != "arm") { deps += [ "//gpu/tools/compositor_model_bench" ] } @@ -66,7 +66,7 @@ vars = { # Three lines of non-changing comments so that # the commit queue can handle CLs rolling PDFium # and whatever else without interference from each other. - 'pdfium_revision': 'b3300162a1ebacc973ff9793029caf4db9a4f5e5', + 'pdfium_revision': 'eddab4425614e49146f904f00da4a664ba4b581b', # Three lines of non-changing comments so that # the commit queue can handle CLs rolling openmax_dl # and whatever else without interference from each other. @@ -274,9 +274,6 @@ 'deep_memory_profiler': { 'filepath': 'tools/(deep_memory_profiler|find_runtime_symbols)', }, - 'device_bluetooth': { - 'filepath': 'device/.*bluetooth' - }, 'device_sensors': { 'filepath': 'content/browser/device_sensors/|'\ 'content/common/device_sensors/|'\ @@ -928,7 +925,6 @@ 'cookie_monster': ['erikwright@chromium.org'], 'custom_handlers': ['vabr+watchlist@chromium.org'], 'deep_memory_profiler': ['dmikurube@chromium.org'], - 'device_bluetooth': ['scheib+watch@chromium.org'], 'device_sensors': ['timvolodine@chromium.org', 'mvanouwerkerk@chromium.org', 'rijubrata.bhaumik@intel.com', diff --git a/ash/content/display/screen_orientation_controller_chromeos.cc b/ash/content/display/screen_orientation_controller_chromeos.cc index dbd4465..07659d7c 100644 --- a/ash/content/display/screen_orientation_controller_chromeos.cc +++ b/ash/content/display/screen_orientation_controller_chromeos.cc @@ -196,11 +196,10 @@ void ScreenOrientationController::Unlock(content::WebContents* web_contents) { void ScreenOrientationController::OnDisplayConfigurationChanged() { if (ignore_display_configuration_updates_) return; - DisplayManager* display_manager = Shell::GetInstance()->display_manager(); - if (!display_manager->HasInternalDisplay()) - return; gfx::Display::Rotation user_rotation = - display_manager->GetDisplayInfo(gfx::Display::InternalDisplayId()) + Shell::GetInstance() + ->display_manager() + ->GetDisplayInfo(gfx::Display::InternalDisplayId()) .rotation(); if (user_rotation != current_rotation_) { // A user may change other display configuration settings. When the user @@ -213,14 +212,11 @@ void ScreenOrientationController::OnDisplayConfigurationChanged() { void ScreenOrientationController::OnMaximizeModeStarted() { DisplayManager* display_manager = Shell::GetInstance()->display_manager(); - // Do not exit early, as the internal display can be determined after Maximize - // Mode has started. (chrome-os-partner:38796) - // Always start observing. - if (display_manager->HasInternalDisplay()) { - current_rotation_ = user_rotation_ = - display_manager->GetDisplayInfo(gfx::Display::InternalDisplayId()) - .rotation(); - } + if (!display_manager->HasInternalDisplay()) + return; + current_rotation_ = user_rotation_ = + display_manager->GetDisplayInfo(gfx::Display::InternalDisplayId()) + .rotation(); if (!rotation_locked_) LoadDisplayRotationProperties(); chromeos::AccelerometerReader::GetInstance()->AddObserver(this); @@ -228,6 +224,8 @@ void ScreenOrientationController::OnMaximizeModeStarted() { } void ScreenOrientationController::OnMaximizeModeEnded() { + if (!Shell::GetInstance()->display_manager()->HasInternalDisplay()) + return; chromeos::AccelerometerReader::GetInstance()->RemoveObserver(this); Shell::GetInstance()->display_controller()->RemoveObserver(this); if (current_rotation_ != user_rotation_) diff --git a/ash/content/display/screen_orientation_controller_chromeos_unittest.cc b/ash/content/display/screen_orientation_controller_chromeos_unittest.cc index 342e958..ca02ca3 100644 --- a/ash/content/display/screen_orientation_controller_chromeos_unittest.cc +++ b/ash/content/display/screen_orientation_controller_chromeos_unittest.cc @@ -589,23 +589,4 @@ TEST_F(ScreenOrientationControllerTest, UserRotationLockDisallowsRotation) { EXPECT_EQ(gfx::Display::ROTATE_0, GetInternalDisplayRotation()); } -// Tests that when MaximizeMode is triggered before the internal display is -// ready, that ScreenOrientationController still begins listening to events, -// which require an internal display to be acted upon. -TEST_F(ScreenOrientationControllerTest, InternalDisplayNotAvailableAtStartup) { - int64 internal_display_id = gfx::Display::InternalDisplayId(); - gfx::Display::SetInternalDisplayId(gfx::Display::kInvalidDisplayID); - - EnableMaximizeMode(true); - - // Should not crash, even thought there is no internal display. - SetInternalDisplayRotation(gfx::Display::ROTATE_180); - EXPECT_FALSE(RotationLocked()); - - // With an internal display now available, functionality should resume. - gfx::Display::SetInternalDisplayId(internal_display_id); - SetInternalDisplayRotation(gfx::Display::ROTATE_90); - EXPECT_TRUE(RotationLocked()); -} - } // namespace ash diff --git a/build/android/buildbot/bb_host_steps.py b/build/android/buildbot/bb_host_steps.py index 1e927fb..6630321 100755 --- a/build/android/buildbot/bb_host_steps.py +++ b/build/android/buildbot/bb_host_steps.py @@ -4,7 +4,6 @@ # found in the LICENSE file. import os -import json import sys import bb_utils @@ -86,9 +85,7 @@ def BisectPerfRegression(options): RunCmd([SrcPath('tools', 'prepare-bisect-perf-regression.py'), '-w', os.path.join(constants.DIR_SOURCE_ROOT, os.pardir)]) RunCmd([SrcPath('tools', 'run-bisect-perf-regression.py'), - '-w', os.path.join(constants.DIR_SOURCE_ROOT, os.pardir), - '--build-properties=%s' % json.dumps(options.build_properties)] + - args) + '-w', os.path.join(constants.DIR_SOURCE_ROOT, os.pardir)] + args) def GetHostStepCmds(): diff --git a/build/gn_migration.gypi b/build/gn_migration.gypi index ad6560c..1dacbb5 100644 --- a/build/gn_migration.gypi +++ b/build/gn_migration.gypi @@ -281,6 +281,7 @@ ['use_x11==1', { 'dependencies': [ + '../media/media.gyp:player_x11', '../tools/xdisplaycheck/xdisplaycheck.gyp:xdisplaycheck', ], 'conditions': [ diff --git a/cc/blink/web_compositor_support_impl.cc b/cc/blink/web_compositor_support_impl.cc index e69abf6..e335ba3 100644 --- a/cc/blink/web_compositor_support_impl.cc +++ b/cc/blink/web_compositor_support_impl.cc @@ -93,6 +93,10 @@ WebScrollbarLayer* WebCompositorSupportImpl::createSolidColorScrollbarLayer( is_left_side_vertical_scrollbar); } +WebDisplayItemList* WebCompositorSupportImpl::createDisplayItemList() { + return new WebDisplayItemListImpl(); +} + WebCompositorAnimation* WebCompositorSupportImpl::createAnimation( const blink::WebCompositorAnimationCurve& curve, blink::WebCompositorAnimation::TargetProperty target, diff --git a/cc/blink/web_compositor_support_impl.h b/cc/blink/web_compositor_support_impl.h index b924be9..4b41691 100644 --- a/cc/blink/web_compositor_support_impl.h +++ b/cc/blink/web_compositor_support_impl.h @@ -42,6 +42,7 @@ class CC_BLINK_EXPORT WebCompositorSupportImpl int thumb_thickness, int track_start, bool is_left_side_vertical_scrollbar); + virtual blink::WebDisplayItemList* createDisplayItemList(); virtual blink::WebCompositorAnimation* createAnimation( const blink::WebCompositorAnimationCurve& curve, blink::WebCompositorAnimation::TargetProperty target, diff --git a/cc/blink/web_content_layer_impl.cc b/cc/blink/web_content_layer_impl.cc index 90d7f9b..92242230 100644 --- a/cc/blink/web_content_layer_impl.cc +++ b/cc/blink/web_content_layer_impl.cc @@ -72,15 +72,16 @@ void WebContentLayerImpl::PaintContents( client_->paintContents(canvas, clip, PaintingControlToWeb(painting_control)); } -void WebContentLayerImpl::PaintContentsToDisplayList( - cc::DisplayItemList* display_list, +scoped_refptr<cc::DisplayItemList> +WebContentLayerImpl::PaintContentsToDisplayList( const gfx::Rect& clip, cc::ContentLayerClient::PaintingControlSetting painting_control) { if (!client_) - return; + return cc::DisplayItemList::Create(); - WebDisplayItemListImpl list(display_list); + WebDisplayItemListImpl list; client_->paintContents(&list, clip, PaintingControlToWeb(painting_control)); + return list.ToDisplayItemList(); } bool WebContentLayerImpl::FillsBoundsCompletely() const { diff --git a/cc/blink/web_content_layer_impl.h b/cc/blink/web_content_layer_impl.h index e7b04b0..3e7b55c 100644 --- a/cc/blink/web_content_layer_impl.h +++ b/cc/blink/web_content_layer_impl.h @@ -39,8 +39,7 @@ class WebContentLayerImpl : public blink::WebContentLayer, void PaintContents(SkCanvas* canvas, const gfx::Rect& clip, PaintingControlSetting painting_control) override; - void PaintContentsToDisplayList( - cc::DisplayItemList* display_list, + scoped_refptr<cc::DisplayItemList> PaintContentsToDisplayList( const gfx::Rect& clip, PaintingControlSetting painting_control) override; bool FillsBoundsCompletely() const override; diff --git a/cc/blink/web_display_item_list_impl.cc b/cc/blink/web_display_item_list_impl.cc index 2bad804..a253beb 100644 --- a/cc/blink/web_display_item_list_impl.cc +++ b/cc/blink/web_display_item_list_impl.cc @@ -25,9 +25,12 @@ namespace cc_blink { -WebDisplayItemListImpl::WebDisplayItemListImpl( - cc::DisplayItemList* display_list) - : display_item_list_(display_list) { +WebDisplayItemListImpl::WebDisplayItemListImpl() + : display_item_list_(cc::DisplayItemList::Create()) { +} + +scoped_refptr<cc::DisplayItemList> WebDisplayItemListImpl::ToDisplayItemList() { + return display_item_list_; } void WebDisplayItemListImpl::appendDrawingItem(const SkPicture* picture) { diff --git a/cc/blink/web_display_item_list_impl.h b/cc/blink/web_display_item_list_impl.h index ff94ee3..abaf221 100644 --- a/cc/blink/web_display_item_list_impl.h +++ b/cc/blink/web_display_item_list_impl.h @@ -31,9 +31,11 @@ namespace cc_blink { class WebDisplayItemListImpl : public blink::WebDisplayItemList { public: - CC_BLINK_EXPORT WebDisplayItemListImpl(cc::DisplayItemList* display_list); + CC_BLINK_EXPORT WebDisplayItemListImpl(); virtual ~WebDisplayItemListImpl(); + scoped_refptr<cc::DisplayItemList> ToDisplayItemList(); + // blink::WebDisplayItemList implementation. virtual void appendDrawingItem(const SkPicture*); virtual void appendClipItem( @@ -61,7 +63,7 @@ class WebDisplayItemListImpl : public blink::WebDisplayItemList { virtual void appendEndScrollItem(); private: - cc::DisplayItemList* display_item_list_; + scoped_refptr<cc::DisplayItemList> display_item_list_; DISALLOW_COPY_AND_ASSIGN(WebDisplayItemListImpl); }; diff --git a/cc/debug/rasterize_and_record_benchmark.cc b/cc/debug/rasterize_and_record_benchmark.cc index f9a48c9..ab05f30 100644 --- a/cc/debug/rasterize_and_record_benchmark.cc +++ b/cc/debug/rasterize_and_record_benchmark.cc @@ -206,11 +206,8 @@ void RasterizeAndRecordBenchmark::RunOnDisplayListLayer( kTimeCheckInterval); do { - const bool use_cached_picture = true; - display_list = - DisplayItemList::Create(visible_layer_rect, use_cached_picture); - painter->PaintContentsToDisplayList( - display_list.get(), visible_layer_rect, painting_control); + display_list = painter->PaintContentsToDisplayList(visible_layer_rect, + painting_control); display_list->CreateAndCacheSkPicture(); if (memory_used) { diff --git a/cc/layers/content_layer_client.h b/cc/layers/content_layer_client.h index 3a82136..4f9c754 100644 --- a/cc/layers/content_layer_client.h +++ b/cc/layers/content_layer_client.h @@ -29,8 +29,7 @@ class CC_EXPORT ContentLayerClient { const gfx::Rect& clip, PaintingControlSetting painting_status) = 0; - virtual void PaintContentsToDisplayList( - DisplayItemList* display_list, + virtual scoped_refptr<DisplayItemList> PaintContentsToDisplayList( const gfx::Rect& clip, PaintingControlSetting painting_status) = 0; diff --git a/cc/layers/picture_image_layer.cc b/cc/layers/picture_image_layer.cc index 2d338f0..b972ec1 100644 --- a/cc/layers/picture_image_layer.cc +++ b/cc/layers/picture_image_layer.cc @@ -63,17 +63,19 @@ void PictureImageLayer::PaintContents( canvas->drawBitmap(bitmap_, 0, 0); } -void PictureImageLayer::PaintContentsToDisplayList( - DisplayItemList* display_list, +scoped_refptr<DisplayItemList> PictureImageLayer::PaintContentsToDisplayList( const gfx::Rect& clip, ContentLayerClient::PaintingControlSetting painting_control) { + scoped_refptr<DisplayItemList> display_item_list = DisplayItemList::Create(); + SkPictureRecorder recorder; SkCanvas* canvas = recorder.beginRecording(gfx::RectToSkRect(clip)); PaintContents(canvas, clip, painting_control); skia::RefPtr<SkPicture> picture = skia::AdoptRef(recorder.endRecordingAsPicture()); - display_list->AppendItem(DrawingDisplayItem::Create(picture)); + display_item_list->AppendItem(DrawingDisplayItem::Create(picture)); + return display_item_list; } bool PictureImageLayer::FillsBoundsCompletely() const { diff --git a/cc/layers/picture_image_layer.h b/cc/layers/picture_image_layer.h index a12a1b9..8d40550 100644 --- a/cc/layers/picture_image_layer.h +++ b/cc/layers/picture_image_layer.h @@ -27,8 +27,7 @@ class CC_EXPORT PictureImageLayer : public PictureLayer, ContentLayerClient { SkCanvas* canvas, const gfx::Rect& clip, ContentLayerClient::PaintingControlSetting painting_control) override; - void PaintContentsToDisplayList( - DisplayItemList* display_list, + scoped_refptr<DisplayItemList> PaintContentsToDisplayList( const gfx::Rect& clip, ContentLayerClient::PaintingControlSetting painting_control) override; bool FillsBoundsCompletely() const override; diff --git a/cc/layers/picture_image_layer_unittest.cc b/cc/layers/picture_image_layer_unittest.cc index cd95c9f..5b9375d 100644 --- a/cc/layers/picture_image_layer_unittest.cc +++ b/cc/layers/picture_image_layer_unittest.cc @@ -33,44 +33,9 @@ TEST(PictureImageLayerTest, PaintContentsToDisplayList) { layer->SetBitmap(image_bitmap); layer->SetBounds(gfx::Size(layer_rect.width(), layer_rect.height())); - bool use_cached_picture = false; scoped_refptr<DisplayItemList> display_list = - DisplayItemList::Create(layer_rect, use_cached_picture); - layer->PaintContentsToDisplayList( - display_list.get(), layer_rect, - ContentLayerClient::PAINTING_BEHAVIOR_NORMAL); - unsigned char actual_pixels[4 * 200 * 200] = {0}; - DrawDisplayList(actual_pixels, layer_rect, display_list); - - EXPECT_EQ(0, memcmp(actual_pixels, image_pixels, 4 * 200 * 200)); -} - -TEST(PictureImageLayerTest, PaintContentsToCachedDisplayList) { - scoped_refptr<PictureImageLayer> layer = PictureImageLayer::Create(); - gfx::Rect layer_rect(200, 200); - - SkBitmap image_bitmap; - unsigned char image_pixels[4 * 200 * 200] = {0}; - SkImageInfo info = - SkImageInfo::MakeN32Premul(layer_rect.width(), layer_rect.height()); - image_bitmap.installPixels(info, image_pixels, info.minRowBytes()); - SkCanvas image_canvas(image_bitmap); - image_canvas.clear(SK_ColorRED); - SkPaint blue_paint; - blue_paint.setColor(SK_ColorBLUE); - image_canvas.drawRectCoords(0.f, 0.f, 100.f, 100.f, blue_paint); - image_canvas.drawRectCoords(100.f, 100.f, 200.f, 200.f, blue_paint); - - layer->SetBitmap(image_bitmap); - layer->SetBounds(gfx::Size(layer_rect.width(), layer_rect.height())); - - bool use_cached_picture = true; - scoped_refptr<DisplayItemList> display_list = - DisplayItemList::Create(layer_rect, use_cached_picture); - layer->PaintContentsToDisplayList( - display_list.get(), layer_rect, - ContentLayerClient::PAINTING_BEHAVIOR_NORMAL); - display_list->CreateAndCacheSkPicture(); + layer->PaintContentsToDisplayList( + layer_rect, ContentLayerClient::PAINTING_BEHAVIOR_NORMAL); unsigned char actual_pixels[4 * 200 * 200] = {0}; DrawDisplayList(actual_pixels, layer_rect, display_list); diff --git a/cc/layers/picture_layer_unittest.cc b/cc/layers/picture_layer_unittest.cc index 917a36f..2f64693 100644 --- a/cc/layers/picture_layer_unittest.cc +++ b/cc/layers/picture_layer_unittest.cc @@ -25,11 +25,11 @@ class MockContentLayerClient : public ContentLayerClient { void PaintContents(SkCanvas* canvas, const gfx::Rect& clip, PaintingControlSetting picture_control) override {} - void PaintContentsToDisplayList( - DisplayItemList* display_list, + scoped_refptr<DisplayItemList> PaintContentsToDisplayList( const gfx::Rect& clip, PaintingControlSetting picture_control) override { NOTIMPLEMENTED(); + return DisplayItemList::Create(); } bool FillsBoundsCompletely() const override { return false; }; }; diff --git a/cc/layers/video_frame_provider.h b/cc/layers/video_frame_provider.h index ff632f6..45e6c411 100644 --- a/cc/layers/video_frame_provider.h +++ b/cc/layers/video_frame_provider.h @@ -6,7 +6,6 @@ #define CC_LAYERS_VIDEO_FRAME_PROVIDER_H_ #include "base/memory/ref_counted.h" -#include "base/time/time.h" #include "cc/base/cc_export.h" namespace media { @@ -15,39 +14,27 @@ class VideoFrame; namespace cc { -// VideoFrameProvider and VideoFrameProvider::Client define the relationship by -// which video frames are exchanged between a provider and client. -// -// Threading notes: This class may be used in a multithreaded manner. However, -// if the Client implementation calls GetCurrentFrame()/PutCurrentFrame() from -// one thread, the provider must ensure that all client methods (except -// StopUsingProvider()) are called from that thread (typically the compositor -// thread). +// Threading notes: This class may be used in a multi threaded manner. +// Specifically, the implementation may call GetCurrentFrame() or +// PutCurrentFrame() from the compositor thread. If so, the caller is +// responsible for making sure Client::DidReceiveFrame() and +// Client::DidUpdateMatrix() are only called from this same thread. class CC_EXPORT VideoFrameProvider { public: + virtual ~VideoFrameProvider() {} + class CC_EXPORT Client { public: - // The provider will call this method to tell the client to stop using it. + // Provider will call this method to tell the client to stop using it. // StopUsingProvider() may be called from any thread. The client should // block until it has PutCurrentFrame() any outstanding frames. virtual void StopUsingProvider() = 0; - // Notifies the client that it should start or stop making regular - // UpdateCurrentFrame() calls to the provider. No further calls to - // UpdateCurrentFrame() should be made once StopRendering() returns. - // - // Callers should use these methods to indicate when it expects and no - // longer expects (respectively) to have new frames for the client. Clients - // may use this information for power conservation. - virtual void StartRendering() = 0; - virtual void StopRendering() = 0; - - // Notifies the client that GetCurrentFrame() will return new data. - // TODO(dalecurtis): Nuke this once VideoFrameProviderClientImpl is using a - // BeginFrameObserver based approach. http://crbug.com/336733 + // Notifies the provider's client that a call to GetCurrentFrame() will + // return new data. virtual void DidReceiveFrame() = 0; - // Notifies the client of a new UV transform matrix to be used. + // Notifies the provider's client of a new UV transform matrix to be used. virtual void DidUpdateMatrix(const float* matrix) = 0; protected: @@ -58,33 +45,18 @@ class CC_EXPORT VideoFrameProvider { // that the provider is not destroyed before this call returns. virtual void SetVideoFrameProviderClient(Client* client) = 0; - // Called by the client on a regular interval. Returns true if a new frame - // will be available via GetCurrentFrame() which should be displayed within - // the presentation interval [|deadline_min|, |deadline_max|]. - // - // Implementations may use this to drive frame acquisition from underlying - // sources, so it must be called by clients before calling GetCurrentFrame(). - virtual bool UpdateCurrentFrame(base::TimeTicks deadline_min, - base::TimeTicks deadline_max) = 0; - - // Returns the current frame, which may have been updated by a recent call to - // UpdateCurrentFrame(). A call to this method does not ensure that the frame - // will be rendered. A subsequent call to PutCurrentFrame() must be made if - // the frame is expected to be rendered. - // - // Clients should call this in response to UpdateCurrentFrame() returning true - // or in response to a DidReceiveFrame() call. - // - // TODO(dalecurtis): Remove text about DidReceiveFrame() once the old path - // has been removed. http://crbug.com/439548 + // This function places a lock on the current frame and returns a pointer to + // it. Calls to this method should always be followed with a call to + // PutCurrentFrame(). + // Only the current provider client should call this function. virtual scoped_refptr<media::VideoFrame> GetCurrentFrame() = 0; - // Indicates that the last frame returned via GetCurrentFrame() is expected to - // be rendered. Must only occur after a previous call to GetCurrentFrame(). - virtual void PutCurrentFrame() = 0; - - protected: - virtual ~VideoFrameProvider() {} + // This function releases the lock on the video frame. It should always be + // called after GetCurrentFrame(). Frames passed into this method + // should no longer be referenced after the call is made. Only the current + // provider client should call this function. + virtual void PutCurrentFrame( + const scoped_refptr<media::VideoFrame>& frame) = 0; }; } // namespace cc diff --git a/cc/layers/video_frame_provider_client_impl.cc b/cc/layers/video_frame_provider_client_impl.cc index 339d4fc..68e10dc 100644 --- a/cc/layers/video_frame_provider_client_impl.cc +++ b/cc/layers/video_frame_provider_client_impl.cc @@ -78,10 +78,11 @@ VideoFrameProviderClientImpl::AcquireLockAndCurrentFrame() { return provider_->GetCurrentFrame(); } -void VideoFrameProviderClientImpl::PutCurrentFrame() { +void VideoFrameProviderClientImpl::PutCurrentFrame( + const scoped_refptr<media::VideoFrame>& frame) { DCHECK(thread_checker_.CalledOnValidThread()); provider_lock_.AssertAcquired(); - provider_->PutCurrentFrame(); + provider_->PutCurrentFrame(frame); } void VideoFrameProviderClientImpl::ReleaseLock() { @@ -103,16 +104,6 @@ void VideoFrameProviderClientImpl::StopUsingProvider() { provider_ = nullptr; } -void VideoFrameProviderClientImpl::StartRendering() { - // TODO(dalecurtis, sunnyps): Hook this method up to control when to start - // observing vsync intervals. http://crbug.com/336733 -} - -void VideoFrameProviderClientImpl::StopRendering() { - // TODO(dalecurtis, sunnyps): Hook this method up to control when to stop - // observing vsync intervals. http://crbug.com/336733 -} - void VideoFrameProviderClientImpl::DidReceiveFrame() { TRACE_EVENT1("cc", "VideoFrameProviderClientImpl::DidReceiveFrame", diff --git a/cc/layers/video_frame_provider_client_impl.h b/cc/layers/video_frame_provider_client_impl.h index ba2eb73..2688ad2 100644 --- a/cc/layers/video_frame_provider_client_impl.h +++ b/cc/layers/video_frame_provider_client_impl.h @@ -37,7 +37,7 @@ class CC_EXPORT VideoFrameProviderClientImpl void Stop(); scoped_refptr<media::VideoFrame> AcquireLockAndCurrentFrame(); - void PutCurrentFrame(); + void PutCurrentFrame(const scoped_refptr<media::VideoFrame>& frame); void ReleaseLock(); const gfx::Transform& StreamTextureMatrix() const; @@ -46,8 +46,6 @@ class CC_EXPORT VideoFrameProviderClientImpl // Called on the main thread. void StopUsingProvider() override; // Called on the impl thread. - void StartRendering() override; - void StopRendering() override; void DidReceiveFrame() override; void DidUpdateMatrix(const float* matrix) override; diff --git a/cc/layers/video_layer_impl.cc b/cc/layers/video_layer_impl.cc index 0e9add0..3ca0978 100644 --- a/cc/layers/video_layer_impl.cc +++ b/cc/layers/video_layer_impl.cc @@ -380,7 +380,7 @@ void VideoLayerImpl::DidDraw(ResourceProvider* resource_provider) { frame_resources_.clear(); } - provider_client_impl_->PutCurrentFrame(); + provider_client_impl_->PutCurrentFrame(frame_); frame_ = nullptr; provider_client_impl_->ReleaseLock(); diff --git a/cc/resources/display_item.cc b/cc/resources/display_item.cc index 33069a53..52bdec1 100644 --- a/cc/resources/display_item.cc +++ b/cc/resources/display_item.cc @@ -9,4 +9,8 @@ namespace cc { DisplayItem::DisplayItem() { } +void DisplayItem::RasterForTracing(SkCanvas* canvas) const { + Raster(canvas, nullptr); +} + } // namespace cc diff --git a/cc/resources/display_item.h b/cc/resources/display_item.h index d065958..ba4e733 100644 --- a/cc/resources/display_item.h +++ b/cc/resources/display_item.h @@ -21,6 +21,7 @@ class CC_EXPORT DisplayItem { virtual void Raster(SkCanvas* canvas, SkDrawPictureCallback* callback) const = 0; + virtual void RasterForTracing(SkCanvas* canvas) const; virtual bool IsSuitableForGpuRasterization() const = 0; virtual int ApproximateOpCount() const = 0; diff --git a/cc/resources/display_item_list.cc b/cc/resources/display_item_list.cc index 0b2e12e..b541c5f 100644 --- a/cc/resources/display_item_list.cc +++ b/cc/resources/display_item_list.cc @@ -20,36 +20,12 @@ namespace cc { -DisplayItemList::DisplayItemList(gfx::Rect layer_rect, bool use_cached_picture) - : recorder_(new SkPictureRecorder()), - use_cached_picture_(use_cached_picture), - retain_individual_display_items_(!use_cached_picture), - layer_rect_(layer_rect), - is_suitable_for_gpu_rasterization_(true), - approximate_op_count_(0) { - if (use_cached_picture_) { - SkRTreeFactory factory; - recorder_.reset(new SkPictureRecorder()); - canvas_ = skia::SharePtr(recorder_->beginRecording( - layer_rect_.width(), layer_rect_.height(), &factory)); - canvas_->translate(-layer_rect_.x(), -layer_rect_.y()); - canvas_->clipRect(gfx::RectToSkRect(layer_rect_)); - - bool tracing_enabled; - TRACE_EVENT_CATEGORY_GROUP_ENABLED( - TRACE_DISABLED_BY_DEFAULT("cc.debug.picture") "," - TRACE_DISABLED_BY_DEFAULT("devtools.timeline.picture"), - &tracing_enabled); - if (tracing_enabled) - retain_individual_display_items_ = true; - } +DisplayItemList::DisplayItemList() + : is_suitable_for_gpu_rasterization_(true), approximate_op_count_(0) { } -scoped_refptr<DisplayItemList> DisplayItemList::Create( - gfx::Rect layer_rect, - bool use_cached_picture) { - return make_scoped_refptr( - new DisplayItemList(layer_rect, use_cached_picture)); +scoped_refptr<DisplayItemList> DisplayItemList::Create() { + return make_scoped_refptr(new DisplayItemList()); } DisplayItemList::~DisplayItemList() { @@ -58,7 +34,7 @@ DisplayItemList::~DisplayItemList() { void DisplayItemList::Raster(SkCanvas* canvas, SkDrawPictureCallback* callback, float contents_scale) const { - if (!use_cached_picture_) { + if (!picture_) { canvas->save(); canvas->scale(contents_scale, contents_scale); for (size_t i = 0; i < items_.size(); ++i) { @@ -86,25 +62,25 @@ void DisplayItemList::Raster(SkCanvas* canvas, } void DisplayItemList::CreateAndCacheSkPicture() { - // Convert to an SkPicture for faster rasterization. - DCHECK(use_cached_picture_); - picture_ = skia::AdoptRef(recorder_->endRecordingAsPicture()); + // Convert to an SkPicture for faster rasterization. Code is identical to + // that in Picture::Record. + SkRTreeFactory factory; + SkPictureRecorder recorder; + skia::RefPtr<SkCanvas> canvas; + canvas = skia::SharePtr(recorder.beginRecording( + layer_rect_.width(), layer_rect_.height(), &factory)); + canvas->translate(-layer_rect_.x(), -layer_rect_.y()); + canvas->clipRect(gfx::RectToSkRect(layer_rect_)); + for (size_t i = 0; i < items_.size(); ++i) + items_[i]->Raster(canvas.get(), NULL); + picture_ = skia::AdoptRef(recorder.endRecordingAsPicture()); DCHECK(picture_); - recorder_.reset(); - canvas_.clear(); } void DisplayItemList::AppendItem(scoped_ptr<DisplayItem> item) { is_suitable_for_gpu_rasterization_ &= item->IsSuitableForGpuRasterization(); approximate_op_count_ += item->ApproximateOpCount(); - - if (use_cached_picture_) { - DCHECK(canvas_); - item->Raster(canvas_.get(), NULL); - } - - if (retain_individual_display_items_) - items_.push_back(item.Pass()); + items_.push_back(item.Pass()); } bool DisplayItemList::IsSuitableForGpuRasterization() const { @@ -150,7 +126,8 @@ DisplayItemList::AsValue() const { recorder.beginRecording(layer_rect_.width(), layer_rect_.height()); canvas->translate(-layer_rect_.x(), -layer_rect_.y()); canvas->clipRect(gfx::RectToSkRect(layer_rect_)); - Raster(canvas, NULL, 1.f); + for (size_t i = 0; i < items_.size(); ++i) + items_[i]->RasterForTracing(canvas); skia::RefPtr<SkPicture> picture = skia::AdoptRef(recorder.endRecordingAsPicture()); diff --git a/cc/resources/display_item_list.h b/cc/resources/display_item_list.h index 0a5d468..f490552 100644 --- a/cc/resources/display_item_list.h +++ b/cc/resources/display_item_list.h @@ -18,15 +18,13 @@ class SkCanvas; class SkDrawPictureCallback; -class SkPictureRecorder; namespace cc { class CC_EXPORT DisplayItemList : public base::RefCountedThreadSafe<DisplayItemList> { public: - static scoped_refptr<DisplayItemList> Create(gfx::Rect layer_rect, - bool use_cached_picture); + static scoped_refptr<DisplayItemList> Create(); void Raster(SkCanvas* canvas, SkDrawPictureCallback* callback, @@ -34,6 +32,9 @@ class CC_EXPORT DisplayItemList void AppendItem(scoped_ptr<DisplayItem> item); + void set_layer_rect(gfx::Rect layer_rect) { layer_rect_ = layer_rect; } + gfx::Rect layer_rect() const { return layer_rect_; } + void CreateAndCacheSkPicture(); bool IsSuitableForGpuRasterization() const; @@ -47,16 +48,11 @@ class CC_EXPORT DisplayItemList void GatherPixelRefs(const gfx::Size& grid_cell_size); private: - DisplayItemList(gfx::Rect layer_rect, bool use_cached_picture); + DisplayItemList(); ~DisplayItemList(); ScopedPtrVector<DisplayItem> items_; skia::RefPtr<SkPicture> picture_; - scoped_ptr<SkPictureRecorder> recorder_; - skia::RefPtr<SkCanvas> canvas_; - bool use_cached_picture_; - bool retain_individual_display_items_; - gfx::Rect layer_rect_; bool is_suitable_for_gpu_rasterization_; int approximate_op_count_; diff --git a/cc/resources/display_item_list_unittest.cc b/cc/resources/display_item_list_unittest.cc index 290b25b..b53b84a 100644 --- a/cc/resources/display_item_list_unittest.cc +++ b/cc/resources/display_item_list_unittest.cc @@ -36,9 +36,7 @@ TEST(DisplayItemListTest, SingleDrawingItem) { SkPaint red_paint; red_paint.setColor(SK_ColorRED); unsigned char pixels[4 * 100 * 100] = {0}; - const bool use_cached_picture = true; - scoped_refptr<DisplayItemList> list = - DisplayItemList::Create(layer_rect, use_cached_picture); + scoped_refptr<DisplayItemList> list = DisplayItemList::Create(); gfx::PointF offset(8.f, 9.f); gfx::RectF recording_rect(offset, layer_rect.size()); @@ -49,7 +47,6 @@ TEST(DisplayItemListTest, SingleDrawingItem) { canvas->drawRectCoords(50.f, 50.f, 75.f, 75.f, blue_paint); picture = skia::AdoptRef(recorder.endRecordingAsPicture()); list->AppendItem(DrawingDisplayItem::Create(picture)); - list->CreateAndCacheSkPicture(); DrawDisplayList(pixels, layer_rect, list); SkBitmap expected_bitmap; @@ -79,9 +76,7 @@ TEST(DisplayItemListTest, ClipItem) { SkPaint red_paint; red_paint.setColor(SK_ColorRED); unsigned char pixels[4 * 100 * 100] = {0}; - const bool use_cached_picture = true; - scoped_refptr<DisplayItemList> list = - DisplayItemList::Create(layer_rect, use_cached_picture); + scoped_refptr<DisplayItemList> list = DisplayItemList::Create(); gfx::PointF first_offset(8.f, 9.f); gfx::RectF first_recording_rect(first_offset, layer_rect.size()); @@ -105,7 +100,6 @@ TEST(DisplayItemListTest, ClipItem) { list->AppendItem(DrawingDisplayItem::Create(picture)); list->AppendItem(EndClipDisplayItem::Create()); - list->CreateAndCacheSkPicture(); DrawDisplayList(pixels, layer_rect, list); @@ -137,9 +131,7 @@ TEST(DisplayItemListTest, TransformItem) { SkPaint red_paint; red_paint.setColor(SK_ColorRED); unsigned char pixels[4 * 100 * 100] = {0}; - const bool use_cached_picture = true; - scoped_refptr<DisplayItemList> list = - DisplayItemList::Create(layer_rect, use_cached_picture); + scoped_refptr<DisplayItemList> list = DisplayItemList::Create(); gfx::PointF first_offset(8.f, 9.f); gfx::RectF first_recording_rect(first_offset, layer_rect.size()); @@ -164,7 +156,6 @@ TEST(DisplayItemListTest, TransformItem) { list->AppendItem(DrawingDisplayItem::Create(picture)); list->AppendItem(EndTransformDisplayItem::Create()); - list->CreateAndCacheSkPicture(); DrawDisplayList(pixels, layer_rect, list); @@ -186,13 +177,11 @@ TEST(DisplayItemListTest, TransformItem) { EXPECT_EQ(0, memcmp(pixels, expected_pixels, 4 * 100 * 100)); } -TEST(DisplayItemListTest, FilterItem) { +TEST(DisplayItemList, FilterItem) { gfx::Rect layer_rect(100, 100); FilterOperations filters; unsigned char pixels[4 * 100 * 100] = {0}; - const bool use_cached_picture = true; - scoped_refptr<DisplayItemList> list = - DisplayItemList::Create(layer_rect, use_cached_picture); + scoped_refptr<DisplayItemList> list = DisplayItemList::Create(); SkBitmap source_bitmap; source_bitmap.allocN32Pixels(50, 50); @@ -217,7 +206,6 @@ TEST(DisplayItemListTest, FilterItem) { gfx::RectF filter_bounds(10.f, 10.f, 50.f, 50.f); list->AppendItem(FilterDisplayItem::Create(filters, filter_bounds)); list->AppendItem(EndFilterDisplayItem::Create()); - list->CreateAndCacheSkPicture(); DrawDisplayList(pixels, layer_rect, list); @@ -248,9 +236,8 @@ TEST(DisplayItemListTest, CompactingItems) { gfx::PointF offset(8.f, 9.f); gfx::RectF recording_rect(offset, layer_rect.size()); - bool use_cached_picture = false; - scoped_refptr<DisplayItemList> list_without_caching = - DisplayItemList::Create(layer_rect, use_cached_picture); + scoped_refptr<DisplayItemList> list = DisplayItemList::Create(); + list->set_layer_rect(ToEnclosingRect(recording_rect)); canvas = skia::SharePtr( recorder.beginRecording(gfx::RectFToSkRect(recording_rect))); @@ -258,16 +245,13 @@ TEST(DisplayItemListTest, CompactingItems) { canvas->drawRectCoords(0.f, 0.f, 60.f, 60.f, red_paint); canvas->drawRectCoords(50.f, 50.f, 75.f, 75.f, blue_paint); picture = skia::AdoptRef(recorder.endRecordingAsPicture()); - list_without_caching->AppendItem(DrawingDisplayItem::Create(picture)); - DrawDisplayList(pixels, layer_rect, list_without_caching); + list->AppendItem(DrawingDisplayItem::Create(picture)); + DrawDisplayList(pixels, layer_rect, list); + + list->CreateAndCacheSkPicture(); unsigned char expected_pixels[4 * 100 * 100] = {0}; - use_cached_picture = true; - scoped_refptr<DisplayItemList> list_with_caching = - DisplayItemList::Create(layer_rect, use_cached_picture); - list_with_caching->AppendItem(DrawingDisplayItem::Create(picture)); - list_with_caching->CreateAndCacheSkPicture(); - DrawDisplayList(expected_pixels, layer_rect, list_with_caching); + DrawDisplayList(expected_pixels, layer_rect, list); EXPECT_EQ(0, memcmp(pixels, expected_pixels, 4 * 100 * 100)); } diff --git a/cc/resources/display_list_recording_source.cc b/cc/resources/display_list_recording_source.cc index 701db9b..7ada580 100644 --- a/cc/resources/display_list_recording_source.cc +++ b/cc/resources/display_list_recording_source.cc @@ -119,18 +119,17 @@ bool DisplayListRecordingSource::UpdateAndExpandInvalidation( } } for (int i = 0; i < repeat_count; ++i) { - const bool use_cached_picture = true; - display_list_ = - DisplayItemList::Create(recorded_viewport_, use_cached_picture); - painter->PaintContentsToDisplayList(display_list_.get(), recorded_viewport_, - painting_control); + display_list_ = painter->PaintContentsToDisplayList(recorded_viewport_, + painting_control); } - display_list_->CreateAndCacheSkPicture(); - + display_list_->set_layer_rect(recorded_viewport_); is_suitable_for_gpu_rasterization_ = display_list_->IsSuitableForGpuRasterization(); + DetermineIfSolidColor(); display_list_->EmitTraceSnapshot(); + + display_list_->CreateAndCacheSkPicture(); if (gather_pixel_refs_) display_list_->GatherPixelRefs(grid_cell_size_); diff --git a/cc/resources/drawing_display_item.cc b/cc/resources/drawing_display_item.cc index 91ab3fb..648f9de 100644 --- a/cc/resources/drawing_display_item.cc +++ b/cc/resources/drawing_display_item.cc @@ -34,6 +34,17 @@ void DrawingDisplayItem::Raster(SkCanvas* canvas, canvas->restore(); } +void DrawingDisplayItem::RasterForTracing(SkCanvas* canvas) const { + canvas->save(); + // The picture debugger in about:tracing doesn't drill down into |drawPicture| + // operations. Calling |playback()| rather than |drawPicture()| causes the + // skia operations in |picture_| to appear individually in the picture + // produced for tracing rather than being hidden inside a drawPicture + // operation. + picture_->playback(canvas); + canvas->restore(); +} + bool DrawingDisplayItem::IsSuitableForGpuRasterization() const { return picture_->suitableForGpuRasterization(NULL); } diff --git a/cc/resources/drawing_display_item.h b/cc/resources/drawing_display_item.h index a3eef77..b45a039 100644 --- a/cc/resources/drawing_display_item.h +++ b/cc/resources/drawing_display_item.h @@ -27,6 +27,7 @@ class CC_EXPORT DrawingDisplayItem : public DisplayItem { } void Raster(SkCanvas* canvas, SkDrawPictureCallback* callback) const override; + void RasterForTracing(SkCanvas* canvas) const override; bool IsSuitableForGpuRasterization() const override; int ApproximateOpCount() const override; diff --git a/cc/test/fake_content_layer_client.cc b/cc/test/fake_content_layer_client.cc index c21d87b..296c891 100644 --- a/cc/test/fake_content_layer_client.cc +++ b/cc/test/fake_content_layer_client.cc @@ -71,15 +71,15 @@ void FakeContentLayerClient::PaintContents( } } -void FakeContentLayerClient::PaintContentsToDisplayList( - DisplayItemList* display_list, +scoped_refptr<DisplayItemList> +FakeContentLayerClient::PaintContentsToDisplayList( const gfx::Rect& clip, PaintingControlSetting painting_control) { SkPictureRecorder recorder; skia::RefPtr<SkCanvas> canvas; skia::RefPtr<SkPicture> picture; - display_list->AppendItem( - ClipDisplayItem::Create(clip, std::vector<SkRRect>())); + scoped_refptr<DisplayItemList> list = DisplayItemList::Create(); + list->AppendItem(ClipDisplayItem::Create(clip, std::vector<SkRRect>())); for (RectPaintVector::const_iterator it = draw_rects_.begin(); it != draw_rects_.end(); ++it) { @@ -90,21 +90,21 @@ void FakeContentLayerClient::PaintContentsToDisplayList( canvas->drawRectCoords(draw_rect.x(), draw_rect.y(), draw_rect.width(), draw_rect.height(), paint); picture = skia::AdoptRef(recorder.endRecordingAsPicture()); - display_list->AppendItem(DrawingDisplayItem::Create(picture)); + list->AppendItem(DrawingDisplayItem::Create(picture)); } for (BitmapVector::const_iterator it = draw_bitmaps_.begin(); it != draw_bitmaps_.end(); ++it) { if (!it->transform.IsIdentity()) { - display_list->AppendItem(TransformDisplayItem::Create(it->transform)); + list->AppendItem(TransformDisplayItem::Create(it->transform)); } canvas = skia::SharePtr( recorder.beginRecording(it->bitmap.width(), it->bitmap.height())); canvas->drawBitmap(it->bitmap, it->point.x(), it->point.y(), &it->paint); picture = skia::AdoptRef(recorder.endRecordingAsPicture()); - display_list->AppendItem(DrawingDisplayItem::Create(picture)); + list->AppendItem(DrawingDisplayItem::Create(picture)); if (!it->transform.IsIdentity()) { - display_list->AppendItem(EndTransformDisplayItem::Create()); + list->AppendItem(EndTransformDisplayItem::Create()); } } @@ -118,12 +118,13 @@ void FakeContentLayerClient::PaintContentsToDisplayList( recorder.beginRecording(gfx::RectFToSkRect(draw_rect))); canvas->drawRect(gfx::RectFToSkRect(draw_rect), paint); picture = skia::AdoptRef(recorder.endRecordingAsPicture()); - display_list->AppendItem(DrawingDisplayItem::Create(picture)); + list->AppendItem(DrawingDisplayItem::Create(picture)); draw_rect.Inset(1, 1); } } - display_list->AppendItem(EndClipDisplayItem::Create()); + list->AppendItem(EndClipDisplayItem::Create()); + return list; } bool FakeContentLayerClient::FillsBoundsCompletely() const { return false; } diff --git a/cc/test/fake_content_layer_client.h b/cc/test/fake_content_layer_client.h index d01bbd1..a4ecc59 100644 --- a/cc/test/fake_content_layer_client.h +++ b/cc/test/fake_content_layer_client.h @@ -40,8 +40,7 @@ class FakeContentLayerClient : public ContentLayerClient { void PaintContents(SkCanvas* canvas, const gfx::Rect& rect, PaintingControlSetting painting_control) override; - void PaintContentsToDisplayList( - DisplayItemList* display_list, + scoped_refptr<DisplayItemList> PaintContentsToDisplayList( const gfx::Rect& clip, PaintingControlSetting painting_control) override; bool FillsBoundsCompletely() const override; diff --git a/cc/test/fake_display_list_recording_source.h b/cc/test/fake_display_list_recording_source.h index 544a022..679d5d6 100644 --- a/cc/test/fake_display_list_recording_source.h +++ b/cc/test/fake_display_list_recording_source.h @@ -40,11 +40,9 @@ class FakeDisplayListRecordingSource : public DisplayListRecordingSource { void Rerecord() { ContentLayerClient::PaintingControlSetting painting_control = ContentLayerClient::PAINTING_BEHAVIOR_NORMAL; - bool use_cached_picture = true; - display_list_ = - DisplayItemList::Create(recorded_viewport_, use_cached_picture); - client_.PaintContentsToDisplayList(display_list_.get(), recorded_viewport_, - painting_control); + display_list_ = client_.PaintContentsToDisplayList(recorded_viewport_, + painting_control); + display_list_->set_layer_rect(recorded_viewport_); display_list_->CreateAndCacheSkPicture(); if (gather_pixel_refs_) display_list_->GatherPixelRefs(grid_cell_size_); diff --git a/cc/test/fake_video_frame_provider.cc b/cc/test/fake_video_frame_provider.cc index 3f4b40b..f008d9f 100644 --- a/cc/test/fake_video_frame_provider.cc +++ b/cc/test/fake_video_frame_provider.cc @@ -14,11 +14,6 @@ FakeVideoFrameProvider::~FakeVideoFrameProvider() { client_->StopUsingProvider(); } -bool FakeVideoFrameProvider::UpdateCurrentFrame(base::TimeTicks deadline_min, - base::TimeTicks deadline_max) { - return false; -} - void FakeVideoFrameProvider::SetVideoFrameProviderClient(Client* client) { client_ = client; } diff --git a/cc/test/fake_video_frame_provider.h b/cc/test/fake_video_frame_provider.h index 53903c9..50b6b27 100644 --- a/cc/test/fake_video_frame_provider.h +++ b/cc/test/fake_video_frame_provider.h @@ -17,10 +17,8 @@ class FakeVideoFrameProvider : public VideoFrameProvider { ~FakeVideoFrameProvider() override; void SetVideoFrameProviderClient(Client* client) override; - bool UpdateCurrentFrame(base::TimeTicks deadline_min, - base::TimeTicks deadline_max) override; scoped_refptr<media::VideoFrame> GetCurrentFrame() override; - void PutCurrentFrame() override {} + void PutCurrentFrame(const scoped_refptr<media::VideoFrame>&) override {} Client* client() { return client_; } diff --git a/cc/test/solid_color_content_layer_client.cc b/cc/test/solid_color_content_layer_client.cc index fe13526..7e97bb8 100644 --- a/cc/test/solid_color_content_layer_client.cc +++ b/cc/test/solid_color_content_layer_client.cc @@ -25,11 +25,12 @@ void SolidColorContentLayerClient::PaintContents( paint); } -void SolidColorContentLayerClient::PaintContentsToDisplayList( - DisplayItemList* display_list, +scoped_refptr<DisplayItemList> +SolidColorContentLayerClient::PaintContentsToDisplayList( const gfx::Rect& clip, PaintingControlSetting painting_control) { NOTIMPLEMENTED(); + return DisplayItemList::Create(); } bool SolidColorContentLayerClient::FillsBoundsCompletely() const { diff --git a/cc/test/solid_color_content_layer_client.h b/cc/test/solid_color_content_layer_client.h index 3068f9ae..1ab4c4d 100644 --- a/cc/test/solid_color_content_layer_client.h +++ b/cc/test/solid_color_content_layer_client.h @@ -19,8 +19,7 @@ class SolidColorContentLayerClient : public ContentLayerClient { void PaintContents(SkCanvas* canvas, const gfx::Rect& rect, PaintingControlSetting painting_control) override; - void PaintContentsToDisplayList( - DisplayItemList* display_list, + scoped_refptr<DisplayItemList> PaintContentsToDisplayList( const gfx::Rect& clip, PaintingControlSetting painting_control) override; bool FillsBoundsCompletely() const override; diff --git a/cc/trees/layer_tree_host_common_unittest.cc b/cc/trees/layer_tree_host_common_unittest.cc index 0046b1a..439d920 100644 --- a/cc/trees/layer_tree_host_common_unittest.cc +++ b/cc/trees/layer_tree_host_common_unittest.cc @@ -60,11 +60,11 @@ class MockContentLayerClient : public ContentLayerClient { void PaintContents(SkCanvas* canvas, const gfx::Rect& clip, PaintingControlSetting picture_control) override {} - void PaintContentsToDisplayList( - DisplayItemList* display_list, + scoped_refptr<DisplayItemList> PaintContentsToDisplayList( const gfx::Rect& clip, PaintingControlSetting picture_control) override { NOTIMPLEMENTED(); + return DisplayItemList::Create(); } bool FillsBoundsCompletely() const override { return false; } }; diff --git a/cc/trees/layer_tree_host_pixeltest_masks.cc b/cc/trees/layer_tree_host_pixeltest_masks.cc index 0cb6aea..97765bd 100644 --- a/cc/trees/layer_tree_host_pixeltest_masks.cc +++ b/cc/trees/layer_tree_host_pixeltest_masks.cc @@ -46,11 +46,11 @@ class MaskContentLayerClient : public ContentLayerClient { } } - void PaintContentsToDisplayList( - DisplayItemList* display_list, + scoped_refptr<DisplayItemList> PaintContentsToDisplayList( const gfx::Rect& clip, PaintingControlSetting picture_control) override { NOTIMPLEMENTED(); + return DisplayItemList::Create(); } private: @@ -303,11 +303,11 @@ class CheckerContentLayerClient : public ContentLayerClient { } } } - void PaintContentsToDisplayList( - DisplayItemList* display_list, + scoped_refptr<DisplayItemList> PaintContentsToDisplayList( const gfx::Rect& clip, PaintingControlSetting picture_control) override { NOTIMPLEMENTED(); + return DisplayItemList::Create(); } private: @@ -334,11 +334,11 @@ class CircleContentLayerClient : public ContentLayerClient { bounds_.width() / 4, paint); } - void PaintContentsToDisplayList( - DisplayItemList* display_list, + scoped_refptr<DisplayItemList> PaintContentsToDisplayList( const gfx::Rect& clip, PaintingControlSetting picture_control) override { NOTIMPLEMENTED(); + return DisplayItemList::Create(); } private: diff --git a/cc/trees/layer_tree_host_unittest.cc b/cc/trees/layer_tree_host_unittest.cc index ff8c5e8..db7bb9c 100644 --- a/cc/trees/layer_tree_host_unittest.cc +++ b/cc/trees/layer_tree_host_unittest.cc @@ -1326,11 +1326,11 @@ class TestOpacityChangeLayerDelegate : public ContentLayerClient { if (test_layer_) test_layer_->SetOpacity(0.f); } - void PaintContentsToDisplayList( - DisplayItemList* display_list, + scoped_refptr<DisplayItemList> PaintContentsToDisplayList( const gfx::Rect& clip, PaintingControlSetting picture_control) override { NOTIMPLEMENTED(); + return DisplayItemList::Create(); } bool FillsBoundsCompletely() const override { return false; } @@ -2791,11 +2791,11 @@ class LayerTreeHostTestChangeLayerPropertiesInPaintContents layer_->SetBounds(gfx::Size(2, 2)); } - void PaintContentsToDisplayList( - DisplayItemList* display_list, + scoped_refptr<DisplayItemList> PaintContentsToDisplayList( const gfx::Rect& clip, PaintingControlSetting picture_control) override { NOTIMPLEMENTED(); + return DisplayItemList::Create(); } bool FillsBoundsCompletely() const override { return false; } diff --git a/chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/FakeServerHelper.java b/chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/FakeServerHelper.java index b25eca1..d2a6d15 100644 --- a/chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/FakeServerHelper.java +++ b/chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/FakeServerHelper.java @@ -71,8 +71,11 @@ public class FakeServerHelper { * Deletes the existing FakeServer. */ public static void deleteFakeServer() { - checkFakeServerInitialized( - "useFakeServer must be called before calling deleteFakeServer."); + if (sNativeFakeServer == 0L) { + throw new IllegalStateException( + "useFakeServer must be called before calling deleteFakeServer."); + } + ThreadUtils.runOnUiThreadBlockingNoException(new Callable<Void>() { @Override public Void call() { @@ -124,8 +127,10 @@ public class FakeServerHelper { * @return whether the number of specified entities exist */ public boolean verifyEntityCountByTypeAndName(int count, ModelType modelType, String name) { - checkFakeServerInitialized( + if (sNativeFakeServer == 0L) { + throw new IllegalStateException( "useFakeServer must be called before data verification."); + } return nativeVerifyEntityCountByTypeAndName(mNativeFakeServerHelperAndroid, sNativeFakeServer, count, modelType.toString(), name); } @@ -139,44 +144,16 @@ public class FakeServerHelper { * @param entitySpecifics the EntitySpecifics proto that represents the entity to inject */ public void injectUniqueClientEntity(String name, EntitySpecifics entitySpecifics) { - checkFakeServerInitialized("useFakeServer must be called before data injection."); + if (sNativeFakeServer == 0L) { + throw new IllegalStateException( + "useFakeServer must be called before data injection."); + } // The protocol buffer is serialized as a byte array because it can be easily deserialized // from this format in native code. nativeInjectUniqueClientEntity(mNativeFakeServerHelperAndroid, sNativeFakeServer, name, MessageNano.toByteArray(entitySpecifics)); } - /** - * Injects a bookmark into the fake Sync server. - * - * @param title the title of the bookmark to inject - * @param url the URL of the bookmark to inject. This String will be passed to the native GURL - * class, so it must be a valid URL under its definition. - * @param parentId the ID of the desired parent bookmark folder - */ - public void injectBookmarkEntity(String title, String url, String parentId) { - checkFakeServerInitialized("useFakeServer must be called before data injection."); - nativeInjectBookmarkEntity(mNativeFakeServerHelperAndroid, sNativeFakeServer, title, url, - parentId); - } - - /** - * Returns the ID of the Bookmark Bar. This value is to be used in conjunction with - * injectBookmarkEntity. - * - * @return the opaque ID of the bookmark bar entity stored in the server - */ - public String getBookmarkBarFolderId() { - checkFakeServerInitialized("useFakeServer must be called before access"); - return nativeGetBookmarkBarFolderId(mNativeFakeServerHelperAndroid, sNativeFakeServer); - } - - private static void checkFakeServerInitialized(String failureMessage) { - if (sNativeFakeServer == 0L) { - throw new IllegalStateException(failureMessage); - } - } - // Native methods. private native long nativeInit(); private native long nativeCreateFakeServer(long nativeFakeServerHelperAndroid); @@ -190,9 +167,4 @@ public class FakeServerHelper { private native void nativeInjectUniqueClientEntity( long nativeFakeServerHelperAndroid, long nativeFakeServer, String name, byte[] serializedEntitySpecifics); - private native void nativeInjectBookmarkEntity( - long nativeFakeServerHelperAndroid, long nativeFakeServer, String title, String url, - String parentId); - private native String nativeGetBookmarkBarFolderId( - long nativeFakeServerHelperAndroid, long nativeFakeServer); } diff --git a/chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTest.java b/chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTest.java index 27226bd..730500c 100644 --- a/chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTest.java +++ b/chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTest.java @@ -291,23 +291,6 @@ public class SyncTest extends ChromeShellTestBase { // injected. This data should be retrieved from the Sync node browser data. } - @LargeTest - @Feature({"Sync"}) - public void testDownloadBookmark() throws InterruptedException { - setupTestAccountAndSignInToSync(FOREIGN_SESSION_TEST_MACHINE_ID); - // 3 bookmark folders exist by default: Bookmarks Bar, Other Bookmarks, Mobile Bookmarks. - assertLocalEntityCount("Bookmarks", 3); - - mFakeServerHelper.injectBookmarkEntity( - "Title", "http://chromium.org", mFakeServerHelper.getBookmarkBarFolderId()); - - SyncTestUtil.triggerSyncAndWaitForCompletion(mContext); - assertLocalEntityCount("Bookmarks", 4); - - // TODO(pvalenzuela): Also verify that the downloaded bookmark matches the one that was - // injected. This data should be retrieved from the Sync node browser data. - } - private void setupTestAccountAndSignInToSync( final String syncClientIdentifier) throws InterruptedException { diff --git a/chrome/browser/chromeos/login/helper.cc b/chrome/browser/chromeos/login/helper.cc index f949875..afe1bf4 100644 --- a/chrome/browser/chromeos/login/helper.cc +++ b/chrome/browser/chromeos/login/helper.cc @@ -153,20 +153,8 @@ content::StoragePartition* GetSigninPartition() { } net::URLRequestContextGetter* GetSigninContext() { - if (StartupUtils::IsWebviewSigninEnabled()) { - content::StoragePartition* signin_partition = GetSigninPartition(); - - // Special case for unit tests. There's no LoginDisplayHost thus no - // webview instance. TODO(nkostylev): Investigate if there's a better - // place to address this like dependency injection. http://crbug.com/477402 - if (!signin_partition && !LoginDisplayHostImpl::default_host()) - return ProfileHelper::GetSigninProfile()->GetRequestContext(); - - if (!signin_partition) - return nullptr; - - return signin_partition->GetURLRequestContext(); - } + if (StartupUtils::IsWebviewSigninEnabled()) + return GetSigninPartition()->GetURLRequestContext(); return ProfileHelper::GetSigninProfile()->GetRequestContext(); } diff --git a/chrome/browser/chromeos/login/hid_detection_browsertest.cc b/chrome/browser/chromeos/login/hid_detection_browsertest.cc index d4e31b2..ebb23d2 100644 --- a/chrome/browser/chromeos/login/hid_detection_browsertest.cc +++ b/chrome/browser/chromeos/login/hid_detection_browsertest.cc @@ -43,15 +43,11 @@ void SetUpBluetoothMock( namespace chromeos { -// Boolean parameter is used to run this test for webview (true) and for -// iframe (false) GAIA sign in. -class HidDetectionTest : public OobeBaseTest, - public testing::WithParamInterface<bool> { +class HidDetectionTest : public OobeBaseTest { public: typedef device::InputServiceLinux::InputDeviceInfo InputDeviceInfo; HidDetectionTest() : weak_ptr_factory_(this) { - set_use_webview(GetParam()); InputServiceProxy::SetThreadIdForTesting(content::BrowserThread::UI); HidDetectionTest::InitInputService(); } @@ -116,17 +112,13 @@ class HidDetectionSkipTest : public HidDetectionTest { } }; -IN_PROC_BROWSER_TEST_P(HidDetectionTest, NoDevicesConnected) { +IN_PROC_BROWSER_TEST_F(HidDetectionTest, NoDevicesConnected) { OobeScreenWaiter(OobeDisplay::SCREEN_OOBE_HID_DETECTION).Wait(); } -IN_PROC_BROWSER_TEST_P(HidDetectionSkipTest, BothDevicesPreConnected) { +IN_PROC_BROWSER_TEST_F(HidDetectionSkipTest, BothDevicesPreConnected) { OobeScreenWaiter(OobeDisplay::SCREEN_OOBE_NETWORK).Wait(); -} -INSTANTIATE_TEST_CASE_P(HidDetectionSuite, HidDetectionTest, testing::Bool()); -INSTANTIATE_TEST_CASE_P(HidDetectionSkipSuite, - HidDetectionSkipTest, - testing::Bool()); +} } // namespace chromeos diff --git a/chrome/browser/chromeos/login/login_browsertest.cc b/chrome/browser/chromeos/login/login_browsertest.cc index 5d1cffa..85880b1 100644 --- a/chrome/browser/chromeos/login/login_browsertest.cc +++ b/chrome/browser/chromeos/login/login_browsertest.cc @@ -5,8 +5,6 @@ #include "ash/shell.h" #include "ash/system/tray/system_tray.h" #include "base/command_line.h" -#include "base/json/json_file_value_serializer.h" -#include "base/path_service.h" #include "base/strings/string_util.h" #include "chrome/browser/chrome_notification_types.h" #include "chrome/browser/chromeos/login/login_manager_test.h" @@ -19,9 +17,7 @@ #include "chrome/browser/profiles/profiles_state.h" #include "chrome/browser/ui/browser.h" #include "chrome/common/chrome_constants.h" -#include "chrome/common/chrome_paths.h" #include "chrome/common/chrome_switches.h" -#include "chrome/common/pref_names.h" #include "chrome/test/base/in_process_browser_test.h" #include "chrome/test/base/interactive_test_utils.h" #include "chrome/test/base/tracing.h" @@ -147,33 +143,6 @@ class LoginTest : public chromeos::LoginManagerTest { } }; -class LoginOfflineTest : public LoginTest { - public: - LoginOfflineTest() {} - ~LoginOfflineTest() override {} - - bool SetUpUserDataDirectory() override { - base::FilePath user_data_dir; - CHECK(PathService::Get(chrome::DIR_USER_DATA, &user_data_dir)); - base::FilePath local_state_path = - user_data_dir.Append(chrome::kLocalStateFilename); - base::DictionaryValue local_state_dict; - - // Set webview disabled flag only when local state file does not exist. - // Otherwise, we break PRE tests that leave state in it. - if (!base::PathExists(local_state_path)) { - // TODO(rsorokin): Fix offline test for webview signin. - // http://crbug.com/475569 - local_state_dict.SetBoolean(prefs::kWebviewSigninDisabled, true); - - CHECK(JSONFileValueSerializer(local_state_path) - .Serialize(local_state_dict)); - } - - return LoginTest::SetUpUserDataDirectory(); - } -}; - // Used to make sure that the system tray is visible and within the screen // bounds after login. void TestSystemTrayIsVisible() { @@ -246,14 +215,14 @@ IN_PROC_BROWSER_TEST_F(LoginSigninTest, WebUIVisible) { ASSERT_TRUE(tracing::EndTracing(&json_events)); } -IN_PROC_BROWSER_TEST_F(LoginOfflineTest, PRE_GaiaAuthOffline) { +IN_PROC_BROWSER_TEST_F(LoginTest, PRE_GaiaAuthOffline) { RegisterUser(kTestUser); chromeos::StartupUtils::MarkOobeCompleted(); chromeos::CrosSettings::Get()->SetBoolean( chromeos::kAccountsPrefShowUserNamesOnSignIn, false); } -IN_PROC_BROWSER_TEST_F(LoginOfflineTest, GaiaAuthOffline) { +IN_PROC_BROWSER_TEST_F(LoginTest, GaiaAuthOffline) { bool show_user; ASSERT_TRUE(chromeos::CrosSettings::Get()->GetBoolean( chromeos::kAccountsPrefShowUserNamesOnSignIn, &show_user)); diff --git a/chrome/browser/chromeos/login/login_manager_test.cc b/chrome/browser/chromeos/login/login_manager_test.cc index bd44832..45a6101 100644 --- a/chrome/browser/chromeos/login/login_manager_test.cc +++ b/chrome/browser/chromeos/login/login_manager_test.cc @@ -5,20 +5,14 @@ #include "chrome/browser/chromeos/login/login_manager_test.h" #include "base/command_line.h" -#include "base/json/json_file_value_serializer.h" -#include "base/path_service.h" #include "base/prefs/scoped_user_pref_update.h" #include "chrome/browser/browser_process.h" #include "chrome/browser/chrome_notification_types.h" #include "chrome/browser/chromeos/login/existing_user_controller.h" #include "chrome/browser/chromeos/login/session/user_session_manager.h" #include "chrome/browser/chromeos/login/session/user_session_manager_test_api.h" -#include "chrome/browser/chromeos/login/startup_utils.h" #include "chrome/browser/chromeos/login/ui/login_display_host_impl.h" #include "chrome/browser/chromeos/login/ui/webui_login_view.h" -#include "chrome/common/chrome_constants.h" -#include "chrome/common/chrome_paths.h" -#include "chrome/common/pref_names.h" #include "chromeos/chromeos_switches.h" #include "chromeos/login/auth/key.h" #include "chromeos/login/auth/user_context.h" @@ -33,8 +27,7 @@ namespace chromeos { LoginManagerTest::LoginManagerTest(bool should_launch_browser) - : use_webview_(false), - should_launch_browser_(should_launch_browser), + : should_launch_browser_(should_launch_browser), web_contents_(NULL) { set_exit_when_last_browser_closes(false); } @@ -68,29 +61,6 @@ void LoginManagerTest::SetUpOnMainThread() { should_launch_browser_); } -bool LoginManagerTest::SetUpUserDataDirectory() { - base::FilePath user_data_dir; - CHECK(PathService::Get(chrome::DIR_USER_DATA, &user_data_dir)); - base::FilePath local_state_path = - user_data_dir.Append(chrome::kLocalStateFilename); - - if (!use_webview()) { - // Set webview disabled flag only when local state file does not exist. - // Otherwise, we break PRE tests that leave state in it. - if (!base::PathExists(local_state_path)) { - base::DictionaryValue local_state_dict; - - // TODO(nkostylev): Fix tests that fail with webview signin. - local_state_dict.SetBoolean(prefs::kWebviewSigninDisabled, true); - - CHECK(JSONFileValueSerializer(local_state_path) - .Serialize(local_state_dict)); - } - } - - return MixinBasedBrowserTest::SetUpUserDataDirectory(); -} - void LoginManagerTest::RegisterUser(const std::string& user_id) { ListPrefUpdate users_pref(g_browser_process->local_state(), "LoggedInUsers"); users_pref->AppendIfNotPresent(new base::StringValue(user_id)); diff --git a/chrome/browser/chromeos/login/login_manager_test.h b/chrome/browser/chromeos/login/login_manager_test.h index ff4a6dd..e2b6921 100644 --- a/chrome/browser/chromeos/login/login_manager_test.h +++ b/chrome/browser/chromeos/login/login_manager_test.h @@ -33,7 +33,6 @@ class LoginManagerTest : public MixinBasedBrowserTest { void SetUpCommandLine(base::CommandLine* command_line) override; void SetUpInProcessBrowserTestFixture() override; void SetUpOnMainThread() override; - bool SetUpUserDataDirectory() override; // Registers the user with the given |user_id| on the device. // This method should be called in PRE_* test. @@ -67,12 +66,6 @@ class LoginManagerTest : public MixinBasedBrowserTest { test::JSChecker& js_checker() { return js_checker_; } - protected: - bool use_webview() { return use_webview_; } - void set_use_webview(bool use_webview) { use_webview_ = use_webview; } - - bool use_webview_; - private: void InitializeWebContents(); diff --git a/chrome/browser/chromeos/login/oobe_browsertest.cc b/chrome/browser/chromeos/login/oobe_browsertest.cc index 2f13bd1..da7e596 100644 --- a/chrome/browser/chromeos/login/oobe_browsertest.cc +++ b/chrome/browser/chromeos/login/oobe_browsertest.cc @@ -5,7 +5,6 @@ #include "base/command_line.h" #include "chrome/browser/chrome_notification_types.h" #include "chrome/browser/chromeos/login/existing_user_controller.h" -#include "chrome/browser/chromeos/login/test/oobe_base_test.h" #include "chrome/browser/chromeos/login/test/oobe_screen_waiter.h" #include "chrome/browser/chromeos/login/ui/login_display_host_impl.h" #include "chrome/browser/chromeos/login/ui/webui_login_display.h" @@ -15,8 +14,8 @@ #include "chrome/common/chrome_switches.h" #include "chrome/test/base/in_process_browser_test.h" #include "chromeos/chromeos_switches.h" -#include "content/public/test/browser_test_utils.h" #include "content/public/test/test_utils.h" +#include "google_apis/gaia/fake_gaia.h" #include "google_apis/gaia/gaia_switches.h" #include "net/test/embedded_test_server/embedded_test_server.h" #include "net/test/embedded_test_server/http_response.h" @@ -28,17 +27,28 @@ using namespace net::test_server; namespace chromeos { -// Boolean parameter is used to run this test for webview (true) and for -// iframe (false) GAIA sign in. -class OobeTest : public OobeBaseTest, public testing::WithParamInterface<bool> { +class OobeTest : public InProcessBrowserTest { public: - OobeTest() { set_use_webview(GetParam()); } + OobeTest() {} ~OobeTest() override {} void SetUpCommandLine(base::CommandLine* command_line) override { - command_line->AppendSwitch(switches::kOobeSkipPostLogin); + command_line->AppendSwitch(chromeos::switches::kLoginManager); + command_line->AppendSwitch(chromeos::switches::kForceLoginManagerInTests); + command_line->AppendSwitchASCII(chromeos::switches::kLoginProfile, "user"); + command_line->AppendSwitchASCII( + ::switches::kAuthExtensionPath, "gaia_auth"); + fake_gaia_.Initialize(); + } + + void SetUpOnMainThread() override { + CHECK(embedded_test_server()->InitializeAndWaitUntilReady()); + embedded_test_server()->RegisterRequestHandler( + base::Bind(&FakeGaia::HandleRequest, base::Unretained(&fake_gaia_))); + LOG(INFO) << "Set up http server at " << embedded_test_server()->base_url(); - OobeBaseTest::SetUpCommandLine(command_line); + base::CommandLine::ForCurrentProcess()->AppendSwitchASCII( + ::switches::kGaiaUrl, embedded_test_server()->base_url().spec()); } void TearDownOnMainThread() override { @@ -48,8 +58,6 @@ class OobeTest : public OobeBaseTest, public testing::WithParamInterface<bool> { base::Bind(&chrome::AttemptExit)); content::RunMessageLoop(); } - - OobeBaseTest::TearDownOnMainThread(); } chromeos::WebUILoginDisplay* GetLoginDisplay() { @@ -67,24 +75,40 @@ class OobeTest : public OobeBaseTest, public testing::WithParamInterface<bool> { } private: + FakeGaia fake_gaia_; DISALLOW_COPY_AND_ASSIGN(OobeTest); }; -IN_PROC_BROWSER_TEST_P(OobeTest, NewUser) { - WaitForGaiaPageLoad(); +IN_PROC_BROWSER_TEST_F(OobeTest, NewUser) { + chromeos::WizardController::SkipPostLoginScreensForTesting(); + chromeos::WizardController* wizard_controller = + chromeos::WizardController::default_controller(); + CHECK(wizard_controller); + wizard_controller->SkipToLoginForTesting(LoginScreenContext()); - content::WindowedNotificationObserver session_start_waiter( - chrome::NOTIFICATION_SESSION_STARTED, - content::NotificationService::AllSources()); + content::WindowedNotificationObserver( + chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE, + content::NotificationService::AllSources()).Wait(); - GetLoginDisplay()->ShowSigninScreenForCreds(OobeBaseTest::kFakeUserEmail, - OobeBaseTest::kFakeUserPassword); + // TODO(glotov): mock GAIA server (test_server()) should support + // username/password configuration. + GetLoginDisplay()->ShowSigninScreenForCreds("username", "password"); - session_start_waiter.Wait(); + content::WindowedNotificationObserver( + chrome::NOTIFICATION_SESSION_STARTED, + content::NotificationService::AllSources()).Wait(); } -IN_PROC_BROWSER_TEST_P(OobeTest, Accelerator) { - WaitForGaiaPageLoad(); +IN_PROC_BROWSER_TEST_F(OobeTest, Accelerator) { + chromeos::WizardController::SkipPostLoginScreensForTesting(); + chromeos::WizardController* wizard_controller = + chromeos::WizardController::default_controller(); + CHECK(wizard_controller); + wizard_controller->SkipToLoginForTesting(LoginScreenContext()); + + content::WindowedNotificationObserver( + chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE, + content::NotificationService::AllSources()).Wait(); gfx::NativeWindow login_window = GetLoginWindowWidget()->GetNativeWindow(); @@ -97,6 +121,4 @@ IN_PROC_BROWSER_TEST_P(OobeTest, Accelerator) { OobeScreenWaiter(OobeDisplay::SCREEN_OOBE_ENROLLMENT).Wait(); } -INSTANTIATE_TEST_CASE_P(OobeSuite, OobeTest, testing::Bool()); - } // namespace chromeos diff --git a/chrome/browser/chromeos/login/proxy_auth_dialog_browsertest.cc b/chrome/browser/chromeos/login/proxy_auth_dialog_browsertest.cc index 3eef72f..eca6b5c 100644 --- a/chrome/browser/chromeos/login/proxy_auth_dialog_browsertest.cc +++ b/chrome/browser/chromeos/login/proxy_auth_dialog_browsertest.cc @@ -57,21 +57,13 @@ class ProxyAuthDialogWaiter : public content::WindowedNotificationObserver { } // namespace -// Boolean parameter is used to run this test for webview (true) and for -// iframe (false) GAIA sign in. -class ProxyAuthOnUserBoardScreenTest - : public LoginManagerTest, - public testing::WithParamInterface<bool> { +class ProxyAuthOnUserBoardScreenTest : public LoginManagerTest { public: ProxyAuthOnUserBoardScreenTest() : LoginManagerTest(true /* should_launch_browser */), proxy_server_(net::SpawnedTestServer::TYPE_BASIC_AUTH_PROXY, net::SpawnedTestServer::kLocalhost, - base::FilePath()) { - // TODO(paulmeyer): Re-enable webview version of this test - // (uncomment this line) once http://crbug.com/452452 is fixed. - // set_use_webview(GetParam()); - } + base::FilePath()) {} ~ProxyAuthOnUserBoardScreenTest() override {} @@ -92,13 +84,13 @@ class ProxyAuthOnUserBoardScreenTest DISALLOW_COPY_AND_ASSIGN(ProxyAuthOnUserBoardScreenTest); }; -IN_PROC_BROWSER_TEST_P(ProxyAuthOnUserBoardScreenTest, +IN_PROC_BROWSER_TEST_F(ProxyAuthOnUserBoardScreenTest, PRE_ProxyAuthDialogOnUserBoardScreen) { RegisterUser("test-user@gmail.com"); StartupUtils::MarkOobeCompleted(); } -IN_PROC_BROWSER_TEST_P(ProxyAuthOnUserBoardScreenTest, +IN_PROC_BROWSER_TEST_F(ProxyAuthOnUserBoardScreenTest, ProxyAuthDialogOnUserBoardScreen) { LoginDisplayHost* login_display_host = LoginDisplayHostImpl::default_host(); WebUILoginView* web_ui_login_view = login_display_host->GetWebUILoginView(); @@ -128,8 +120,4 @@ IN_PROC_BROWSER_TEST_P(ProxyAuthOnUserBoardScreenTest, } } -INSTANTIATE_TEST_CASE_P(ProxyAuthOnUserBoardScreenTestSuite, - ProxyAuthOnUserBoardScreenTest, - testing::Bool()); - } // namespace chromeos diff --git a/chrome/browser/chromeos/login/saml/saml_browsertest.cc b/chrome/browser/chromeos/login/saml/saml_browsertest.cc index ce4d3c0..0a11e4c 100644 --- a/chrome/browser/chromeos/login/saml/saml_browsertest.cc +++ b/chrome/browser/chromeos/login/saml/saml_browsertest.cc @@ -170,7 +170,7 @@ void FakeSamlIdp::SetUp(const std::string& base_path, const GURL& gaia_url) { ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir)); html_template_dir_ = test_data_dir.Append("login"); - login_path_ = base_path; + login_path_= base_path; login_auth_path_ = base_path + "Auth"; gaia_assertion_url_ = gaia_url.Resolve("/SSO"); } @@ -259,14 +259,9 @@ scoped_ptr<HttpResponse> FakeSamlIdp::BuildHTMLResponse( } // namespace -// Boolean parameter is used to run this test for webview (true) and for -// iframe (false) GAIA sign in. class SamlTest : public OobeBaseTest, public testing::WithParamInterface<bool> { public: - SamlTest() : saml_load_injected_(false) { - set_use_webview(GetParam()); - set_initialize_fake_merge_session(false); - } + SamlTest() : saml_load_injected_(false) { use_webview_ = GetParam(); } ~SamlTest() override {} void SetUpCommandLine(base::CommandLine* command_line) override { @@ -370,13 +365,13 @@ IN_PROC_BROWSER_TEST_P(SamlTest, SamlUI) { // Saml flow UI expectations. JsExpect("$('gaia-signin').classList.contains('full-width')"); - if (!use_webview()) { + if (!use_webview_) { JsExpect("!$('cancel-add-user-button').hidden"); } // Click on 'cancel'. content::DOMMessageQueue message_queue; // Observe before 'cancel'. - if (use_webview()) { + if (use_webview_) { ASSERT_TRUE(content::ExecuteScript( GetLoginUI()->GetWebContents(), "$('close-button-item').click();")); @@ -402,7 +397,7 @@ IN_PROC_BROWSER_TEST_P(SamlTest, CredentialPassingAPI) { // webview.executeScript and there is no way to control the injection time. // As a result, this test is flaky and fails about 20% of the time. // TODO(xiyuan): Re-enable when webview.addContentScript API is ready. - if (use_webview()) + if (use_webview_) return; fake_saml_idp()->SetLoginHTMLTemplate("saml_api_login.html"); diff --git a/chrome/browser/chromeos/login/session/user_session_manager.cc b/chrome/browser/chromeos/login/session/user_session_manager.cc index 50f75fb..31395c2 100644 --- a/chrome/browser/chromeos/login/session/user_session_manager.cc +++ b/chrome/browser/chromeos/login/session/user_session_manager.cc @@ -997,29 +997,14 @@ void UserSessionManager::UserProfileInitialized(Profile* profile, // empty if |transfer_saml_auth_cookies_on_subsequent_login| is true. const bool transfer_auth_cookies_and_channel_ids_on_first_login = has_auth_cookies_; - - net::URLRequestContextGetter* auth_request_context = - GetAuthRequestContext(); - - // Authentication request context may be missing especially if user didn't - // sign in using GAIA (webview) and webview didn't yet initialize. - if (auth_request_context) { - ProfileAuthData::Transfer( - auth_request_context, profile->GetRequestContext(), - transfer_auth_cookies_and_channel_ids_on_first_login, - transfer_saml_auth_cookies_on_subsequent_login, - base::Bind( - &UserSessionManager::CompleteProfileCreateAfterAuthTransfer, - AsWeakPtr(), profile)); - } else { - // We need to post task so that OnProfileCreated() caller sends out - // NOTIFICATION_PROFILE_CREATED which marks user profile as initialized. - base::MessageLoopProxy::current()->PostTask( - FROM_HERE, - base::Bind( - &UserSessionManager::CompleteProfileCreateAfterAuthTransfer, - AsWeakPtr(), profile)); - } + ProfileAuthData::Transfer( + GetAuthRequestContext(), + profile->GetRequestContext(), + transfer_auth_cookies_and_channel_ids_on_first_login, + transfer_saml_auth_cookies_on_subsequent_login, + base::Bind(&UserSessionManager::CompleteProfileCreateAfterAuthTransfer, + AsWeakPtr(), + profile)); return; } @@ -1246,18 +1231,9 @@ void UserSessionManager::RestoreAuthSessionImpl( OAuth2LoginManagerFactory::GetInstance()->GetForProfile(profile); login_manager->AddObserver(this); - net::URLRequestContextGetter* auth_request_context = GetAuthRequestContext(); - - // Authentication request context may not be available if user was not - // signing in with GAIA webview (i.e. webview instance hasn't been - // initialized at all). Use fallback request context. - if (!auth_request_context) { - auth_request_context = - authenticator_->authentication_context()->GetRequestContext(); - } - login_manager->RestoreSession(auth_request_context, session_restore_strategy_, - user_context_.GetRefreshToken(), - user_context_.GetAuthCode()); + login_manager->RestoreSession( + GetAuthRequestContext(), session_restore_strategy_, + user_context_.GetRefreshToken(), user_context_.GetAuthCode()); } void UserSessionManager::InitRlzImpl(Profile* profile, bool disabled) { @@ -1446,15 +1422,13 @@ void UserSessionManager::UpdateEasyUnlockKeys(const UserContext& user_context) { net::URLRequestContextGetter* UserSessionManager::GetAuthRequestContext() const { - net::URLRequestContextGetter* auth_request_context = nullptr; + net::URLRequestContextGetter* auth_request_context = NULL; if (StartupUtils::IsWebviewSigninEnabled()) { // Webview uses different partition storage than iframe. We need to get // cookies from the right storage for url request to get auth token into // session. - content::StoragePartition* signin_partition = login::GetSigninPartition(); - if (signin_partition) - auth_request_context = signin_partition->GetURLRequestContext(); + auth_request_context = login::GetSigninPartition()->GetURLRequestContext(); } else if (authenticator_.get() && authenticator_->authentication_context()) { auth_request_context = authenticator_->authentication_context()->GetRequestContext(); diff --git a/chrome/browser/chromeos/login/signin/oauth2_browsertest.cc b/chrome/browser/chromeos/login/signin/oauth2_browsertest.cc index c09e3a7..48bdb7b 100644 --- a/chrome/browser/chromeos/login/signin/oauth2_browsertest.cc +++ b/chrome/browser/chromeos/login/signin/oauth2_browsertest.cc @@ -13,7 +13,6 @@ #include "chrome/browser/chromeos/login/signin/oauth2_login_manager.h" #include "chrome/browser/chromeos/login/signin/oauth2_login_manager_factory.h" #include "chrome/browser/chromeos/login/signin_specifics.h" -#include "chrome/browser/chromeos/login/startup_utils.h" #include "chrome/browser/chromeos/login/test/oobe_base_test.h" #include "chrome/browser/chromeos/login/wizard_controller.h" #include "chrome/browser/profiles/profile_manager.h" @@ -131,12 +130,9 @@ class OAuth2LoginManagerStateWaiter : public OAuth2LoginManager::Observer { } // namespace -// Boolean parameter is used to run this test for webview (true) and for -// iframe (false) GAIA sign in. -class OAuth2Test : public OobeBaseTest, - public testing::WithParamInterface<bool> { +class OAuth2Test : public OobeBaseTest { protected: - OAuth2Test() { set_use_webview(GetParam()); } + OAuth2Test() {} void SetUpCommandLine(base::CommandLine* command_line) override { OobeBaseTest::SetUpCommandLine(command_line); @@ -331,17 +327,23 @@ class OAuth2Test : public OobeBaseTest, void StartNewUserSession(bool wait_for_merge) { SetupGaiaServerForNewAccount(); SimulateNetworkOnline(); - WaitForGaiaPageLoad(); + chromeos::WizardController::SkipPostLoginScreensForTesting(); + chromeos::WizardController* wizard_controller = + chromeos::WizardController::default_controller(); + wizard_controller->SkipToLoginForTesting(LoginScreenContext()); - content::WindowedNotificationObserver session_start_waiter( - chrome::NOTIFICATION_SESSION_STARTED, - content::NotificationService::AllSources()); + content::WindowedNotificationObserver( + chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE, + content::NotificationService::AllSources()).Wait(); // Use capitalized and dotted user name on purpose to make sure // our email normalization kicks in. GetLoginDisplay()->ShowSigninScreenForCreds(kTestRawAccountId, kTestAccountPassword); - session_start_waiter.Wait(); + + content::WindowedNotificationObserver( + chrome::NOTIFICATION_SESSION_STARTED, + content::NotificationService::AllSources()).Wait(); if (wait_for_merge) { // Wait for the session merge to finish. @@ -412,7 +414,7 @@ class CookieReader : public base::RefCountedThreadSafe<CookieReader> { }; // PRE_MergeSession is testing merge session for a new profile. -IN_PROC_BROWSER_TEST_P(OAuth2Test, PRE_PRE_PRE_MergeSession) { +IN_PROC_BROWSER_TEST_F(OAuth2Test, PRE_PRE_PRE_MergeSession) { StartNewUserSession(true); // Check for existance of refresh token. ProfileOAuth2TokenService* token_service = @@ -422,6 +424,7 @@ IN_PROC_BROWSER_TEST_P(OAuth2Test, PRE_PRE_PRE_MergeSession) { EXPECT_EQ(GetOAuthStatusFromLocalState(kTestAccountId), user_manager::User::OAUTH2_TOKEN_STATUS_VALID); + scoped_refptr<CookieReader> cookie_reader(new CookieReader()); cookie_reader->ReadCookies(profile()); EXPECT_EQ(cookie_reader->GetCookieValue("SID"), kTestSessionSIDCookie); @@ -432,7 +435,7 @@ IN_PROC_BROWSER_TEST_P(OAuth2Test, PRE_PRE_PRE_MergeSession) { // that was generated in PRE_PRE_PRE_MergeSession test. In this test, we // are not running /MergeSession process since the /ListAccounts call confirms // that the session is not stale. -IN_PROC_BROWSER_TEST_P(OAuth2Test, PRE_PRE_MergeSession) { +IN_PROC_BROWSER_TEST_F(OAuth2Test, PRE_PRE_MergeSession) { SetupGaiaServerForUnexpiredAccount(); SimulateNetworkOnline(); LoginAsExistingUser(); @@ -446,7 +449,7 @@ IN_PROC_BROWSER_TEST_P(OAuth2Test, PRE_PRE_MergeSession) { // MergeSession test is running merge session process for an existing profile // that was generated in PRE_PRE_MergeSession test. -IN_PROC_BROWSER_TEST_P(OAuth2Test, PRE_MergeSession) { +IN_PROC_BROWSER_TEST_F(OAuth2Test, PRE_MergeSession) { SetupGaiaServerForExpiredAccount(); SimulateNetworkOnline(); LoginAsExistingUser(); @@ -462,7 +465,7 @@ IN_PROC_BROWSER_TEST_P(OAuth2Test, PRE_MergeSession) { // MergeSession test is attempting to merge session for an existing profile // that was generated in PRE_PRE_MergeSession test. This attempt should fail // since FakeGaia instance isn't configured to return relevant tokens/cookies. -IN_PROC_BROWSER_TEST_P(OAuth2Test, MergeSession) { +IN_PROC_BROWSER_TEST_F(OAuth2Test, MergeSession) { SimulateNetworkOnline(); content::WindowedNotificationObserver( @@ -532,7 +535,9 @@ class FakeGoogle { } // True if we have already served the test page. - bool IsPageRequested() { return start_event_.IsSignaled(); } + bool IsPageRequested () { + return start_event_.IsSignaled(); + } // Waits until we receive a request to serve the test page. void WaitForPageRequest() { @@ -694,7 +699,7 @@ Browser* FindOrCreateVisibleBrowser(Profile* profile) { return browser; } -IN_PROC_BROWSER_TEST_P(MergeSessionTest, PageThrottle) { +IN_PROC_BROWSER_TEST_F(MergeSessionTest, PageThrottle) { StartNewUserSession(false); // Try to open a page from google.com. @@ -737,7 +742,7 @@ IN_PROC_BROWSER_TEST_P(MergeSessionTest, PageThrottle) { DVLOG(1) << "Loaded page at the end : " << title; } -IN_PROC_BROWSER_TEST_P(MergeSessionTest, XHRThrottle) { +IN_PROC_BROWSER_TEST_F(MergeSessionTest, XHRThrottle) { StartNewUserSession(false); // Wait until we get send merge session request. @@ -791,7 +796,4 @@ IN_PROC_BROWSER_TEST_P(MergeSessionTest, XHRThrottle) { EXPECT_TRUE(fake_google_.IsPageRequested()); } -INSTANTIATE_TEST_CASE_P(OAuth2Suite, OAuth2Test, testing::Bool()); -INSTANTIATE_TEST_CASE_P(MergeSessionSuite, MergeSessionTest, testing::Bool()); - } // namespace chromeos diff --git a/chrome/browser/chromeos/login/startup_utils.cc b/chrome/browser/chromeos/login/startup_utils.cc index c136ea3..1a0d47f 100644 --- a/chrome/browser/chromeos/login/startup_utils.cc +++ b/chrome/browser/chromeos/login/startup_utils.cc @@ -13,7 +13,6 @@ #include "base/sys_info.h" #include "base/threading/thread_restrictions.h" #include "chrome/browser/browser_process.h" -#include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h" #include "chrome/common/chrome_paths.h" #include "chrome/common/extensions/features/feature_channel.h" #include "chrome/common/pref_names.h" @@ -60,7 +59,7 @@ void StartupUtils::RegisterPrefs(PrefRegistrySimple* registry) { registry->RegisterBooleanPref(prefs::kEnrollmentRecoveryRequired, false); registry->RegisterStringPref(prefs::kInitialLocale, "en-US"); registry->RegisterBooleanPref(prefs::kNewOobe, false); - registry->RegisterBooleanPref(prefs::kWebviewSigninDisabled, false); + registry->RegisterBooleanPref(prefs::kWebviewSigninEnabled, false); } // static @@ -180,41 +179,25 @@ std::string StartupUtils::GetInitialLocale() { // static bool StartupUtils::IsWebviewSigninAllowed() { - return !base::CommandLine::ForCurrentProcess()->HasSwitch( - switches::kDisableWebviewSigninFlow); + return extensions::GetCurrentChannel() <= chrome::VersionInfo::CHANNEL_DEV && + !base::CommandLine::ForCurrentProcess()->HasSwitch( + switches::kDisableWebviewSigninFlow); } // static bool StartupUtils::IsWebviewSigninEnabled() { - policy::DeviceCloudPolicyManagerChromeOS* policy_manager = - g_browser_process - ? g_browser_process->platform_part() - ->browser_policy_connector_chromeos() - ->GetDeviceCloudPolicyManager() - : nullptr; - - bool is_remora_or_shark_requisition = - policy_manager - ? policy_manager->IsRemoraRequisition() || - policy_manager->IsSharkRequisition() - : false; - - bool is_webview_disabled_pref = g_browser_process->local_state()->GetBoolean( - prefs::kWebviewSigninDisabled); - - // TODO(dzhioev): Re-enable webview signin for remora/shark requisition - // http://crbug.com/464049 - return !is_remora_or_shark_requisition && IsWebviewSigninAllowed() && - !is_webview_disabled_pref; + return IsWebviewSigninAllowed() && + g_browser_process->local_state()->GetBoolean( + prefs::kWebviewSigninEnabled); } // static bool StartupUtils::EnableWebviewSignin(bool is_enabled) { - if (is_enabled && !IsWebviewSigninAllowed()) + if (!IsWebviewSigninAllowed()) return false; - g_browser_process->local_state()->SetBoolean(prefs::kWebviewSigninDisabled, - !is_enabled); + g_browser_process->local_state()->SetBoolean(prefs::kWebviewSigninEnabled, + is_enabled); return true; } diff --git a/chrome/browser/chromeos/login/test/oobe_base_test.cc b/chrome/browser/chromeos/login/test/oobe_base_test.cc index 5aa0eff..5dcc635 100644 --- a/chrome/browser/chromeos/login/test/oobe_base_test.cc +++ b/chrome/browser/chromeos/login/test/oobe_base_test.cc @@ -20,7 +20,6 @@ #include "chrome/common/pref_names.h" #include "chromeos/chromeos_switches.h" #include "chromeos/dbus/fake_shill_manager_client.h" -#include "components/policy/core/common/policy_switches.h" #include "components/user_manager/fake_user_manager.h" #include "content/public/browser/notification_observer.h" #include "content/public/browser/notification_registrar.h" @@ -33,19 +32,12 @@ namespace chromeos { -// static -const char OobeBaseTest::kFakeUserEmail[] = "fake-email@gmail.com"; -const char OobeBaseTest::kFakeUserPassword[] = "fake-password"; -const char OobeBaseTest::kFakeSIDCookie[] = "fake-SID-cookie"; -const char OobeBaseTest::kFakeLSIDCookie[] = "fake-LSID-cookie"; - OobeBaseTest::OobeBaseTest() : fake_gaia_(new FakeGaia()), network_portal_detector_(NULL), needs_background_networking_(false), gaia_frame_parent_("signin-frame"), - use_webview_(false), - initialize_fake_merge_session_(true) { + use_webview_(false) { set_exit_when_last_browser_closes(false); set_chromeos_user_ = false; } @@ -58,8 +50,6 @@ void OobeBaseTest::SetUp() { PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir); embedded_test_server()->ServeFilesFromDirectory(test_data_dir); - RegisterAdditionalRequestHandlers(); - embedded_test_server()->RegisterRequestHandler( base::Bind(&FakeGaia::HandleRequest, base::Unretained(fake_gaia_.get()))); @@ -77,19 +67,25 @@ void OobeBaseTest::SetUp() { } bool OobeBaseTest::SetUpUserDataDirectory() { - base::FilePath user_data_dir; - CHECK(PathService::Get(chrome::DIR_USER_DATA, &user_data_dir)); - base::FilePath local_state_path = - user_data_dir.Append(chrome::kLocalStateFilename); + if (use_webview_) { + // Fake Dev channel to enable webview signin. + scoped_channel_.reset( + new extensions::ScopedCurrentChannel(chrome::VersionInfo::CHANNEL_DEV)); - if (!use_webview()) { - // Set webview disabled flag only when local state file does not exist. + base::FilePath user_data_dir; + CHECK(PathService::Get(chrome::DIR_USER_DATA, &user_data_dir)); + base::FilePath local_state_path = + user_data_dir.Append(chrome::kLocalStateFilename); + + // Set webview enabled flag only when local state file does not exist. // Otherwise, we break PRE tests that leave state in it. if (!base::PathExists(local_state_path)) { base::DictionaryValue local_state_dict; - - // TODO(nkostylev): Fix tests that fail with webview signin. - local_state_dict.SetBoolean(prefs::kWebviewSigninDisabled, true); + local_state_dict.SetBoolean(prefs::kWebviewSigninEnabled, true); + // OobeCompleted to skip controller-pairing-screen which still uses + // iframe and ends up in a JS error in oobe page init. + // See http://crbug.com/467147 + local_state_dict.SetBoolean(prefs::kOobeComplete, true); CHECK(JSONFileValueSerializer(local_state_path) .Serialize(local_state_dict)); @@ -110,11 +106,6 @@ void OobeBaseTest::SetUpInProcessBrowserTestFixture() { } void OobeBaseTest::SetUpOnMainThread() { - if (initialize_fake_merge_session()) { - fake_gaia_->SetFakeMergeSessionParams(kFakeUserEmail, kFakeSIDCookie, - kFakeLSIDCookie); - } - // Restart the thread as the sandbox host process has already been spawned. embedded_test_server()->RestartThreadAndListen(); @@ -132,7 +123,6 @@ void OobeBaseTest::TearDownOnMainThread() { base::Bind(&chrome::AttemptExit)); content::RunMessageLoop(); } - EXPECT_TRUE(embedded_test_server()->ShutdownAndWaitUntilComplete()); ExtensionApiTest::TearDownOnMainThread(); } @@ -146,14 +136,14 @@ void OobeBaseTest::SetUpCommandLine(base::CommandLine* command_line) { command_line->AppendSwitch(::switches::kDisableBackgroundNetworking); command_line->AppendSwitchASCII(chromeos::switches::kLoginProfile, "user"); - GURL gaia_url = gaia_https_forwarder_->GetURL(std::string()); + GURL gaia_url = gaia_https_forwarder_->GetURL(""); command_line->AppendSwitchASCII(::switches::kGaiaUrl, gaia_url.spec()); command_line->AppendSwitchASCII(::switches::kLsoUrl, gaia_url.spec()); command_line->AppendSwitchASCII(::switches::kGoogleApisUrl, gaia_url.spec()); fake_gaia_->Initialize(); - fake_gaia_->set_issue_oauth_code_cookie(use_webview()); + fake_gaia_->set_issue_oauth_code_cookie(use_webview_); } void OobeBaseTest::InitHttpsForwarders() { @@ -162,9 +152,6 @@ void OobeBaseTest::InitHttpsForwarders() { ASSERT_TRUE(gaia_https_forwarder_->Start()); } -void OobeBaseTest::RegisterAdditionalRequestHandlers() { -} - void OobeBaseTest::SimulateNetworkOffline() { NetworkPortalDetector::CaptivePortalState offline_state; offline_state.status = NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_OFFLINE; @@ -230,32 +217,11 @@ WebUILoginDisplay* OobeBaseTest::GetLoginDisplay() { controller->login_display()); } -void OobeBaseTest::WaitForGaiaPageLoad() { - WaitForSigninScreen(); - - if (!use_webview()) - return; - - ASSERT_TRUE(content::ExecuteScript( - GetLoginUI()->GetWebContents(), - "$('gaia-signin').gaiaAuthHost_.addEventListener('ready'," - "function() {" - "window.domAutomationController.setAutomationId(0);" - "window.domAutomationController.send('GaiaReady');" - "});")); - - content::DOMMessageQueue message_queue; - std::string message; - do { - ASSERT_TRUE(message_queue.WaitForMessage(&message)); - } while (message != "\"GaiaReady\""); -} - void OobeBaseTest::WaitForSigninScreen() { WizardController* wizard_controller = WizardController::default_controller(); - if (wizard_controller) + if (wizard_controller) { wizard_controller->SkipToLoginForTesting(LoginScreenContext()); - + } WizardController::SkipPostLoginScreensForTesting(); login_screen_load_observer_->Wait(); diff --git a/chrome/browser/chromeos/login/test/oobe_base_test.h b/chrome/browser/chromeos/login/test/oobe_base_test.h index 7c2f592..6933a01 100644 --- a/chrome/browser/chromeos/login/test/oobe_base_test.h +++ b/chrome/browser/chromeos/login/test/oobe_base_test.h @@ -34,22 +34,9 @@ class NetworkPortalDetectorTestImpl; // Base class for OOBE, login, SAML and Kiosk tests. class OobeBaseTest : public ExtensionApiTest { public: - // Default fake user email and password, may be used by tests. - - static const char kFakeUserEmail[]; - static const char kFakeUserPassword[]; - - // FakeGaia is configured to return these cookies for kFakeUserEmail. - static const char kFakeSIDCookie[]; - static const char kFakeLSIDCookie[]; - OobeBaseTest(); ~OobeBaseTest() override; - // Subclasses may register their own custom request handlers that will - // process requests prior it gets handled by FakeGaia instance. - virtual void RegisterAdditionalRequestHandlers(); - protected: // InProcessBrowserTest overrides. void SetUp() override; @@ -73,23 +60,12 @@ class OobeBaseTest : public ExtensionApiTest { // Checks JavaScript |expression| in login screen. void JsExpect(const std::string& expression); - bool use_webview() { return use_webview_; } - void set_use_webview(bool use_webview) { use_webview_ = use_webview; } - - bool initialize_fake_merge_session() { - return initialize_fake_merge_session_; - } - void set_initialize_fake_merge_session(bool value) { - initialize_fake_merge_session_ = value; - } - // Returns chrome://oobe WebUI. content::WebUI* GetLoginUI(); // Returns login display. WebUILoginDisplay* GetLoginDisplay(); - void WaitForGaiaPageLoad(); void WaitForSigninScreen(); void ExecuteJsInSigninFrame(const std::string& js); void SetSignFormField(const std::string& field_id, @@ -107,7 +83,6 @@ class OobeBaseTest : public ExtensionApiTest { scoped_ptr<HTTPSForwarder> gaia_https_forwarder_; std::string gaia_frame_parent_; bool use_webview_; - bool initialize_fake_merge_session_; DISALLOW_COPY_AND_ASSIGN(OobeBaseTest); }; diff --git a/chrome/browser/chromeos/login/webview_login_browsertest.cc b/chrome/browser/chromeos/login/webview_login_browsertest.cc index b7d3caa..c3163b2 100644 --- a/chrome/browser/chromeos/login/webview_login_browsertest.cc +++ b/chrome/browser/chromeos/login/webview_login_browsertest.cc @@ -10,27 +10,57 @@ #include "content/public/test/test_utils.h" namespace chromeos { +namespace { +const char kFakeUserEmail[] = "fake-email@gmail.com"; +const char kFakeUserPassword[] = "fake-password"; +const char kFakeSIDCookie[] = "fake-SID-cookie"; +const char kFakeLSIDCookie[] = "fake-LSID-cookie"; +} class WebviewLoginTest : public OobeBaseTest { public: - WebviewLoginTest() { set_use_webview(true); } + WebviewLoginTest() { use_webview_ = true; } ~WebviewLoginTest() override {} + void SetUpOnMainThread() override { + fake_gaia_->SetFakeMergeSessionParams(kFakeUserEmail, kFakeSIDCookie, + kFakeLSIDCookie); + + OobeBaseTest::SetUpOnMainThread(); + } + void SetUpCommandLine(base::CommandLine* command_line) override { command_line->AppendSwitch(switches::kOobeSkipPostLogin); OobeBaseTest::SetUpCommandLine(command_line); } + void WaitForGaiaPageLoaded() { + WaitForSigninScreen(); + + ASSERT_TRUE(content::ExecuteScript( + GetLoginUI()->GetWebContents(), + "$('gaia-signin').gaiaAuthHost_.addEventListener('ready'," + "function() {" + "window.domAutomationController.setAutomationId(0);" + "window.domAutomationController.send('GaiaReady');" + "});")); + + content::DOMMessageQueue message_queue; + std::string message; + ASSERT_TRUE(message_queue.WaitForMessage(&message)); + EXPECT_EQ("\"GaiaReady\"", message); + } + private: DISALLOW_COPY_AND_ASSIGN(WebviewLoginTest); }; IN_PROC_BROWSER_TEST_F(WebviewLoginTest, Basic) { - WaitForGaiaPageLoad(); + WaitForGaiaPageLoaded(); JsExpect("$('close-button-item').hidden"); - SetSignFormField("identifier", OobeBaseTest::kFakeUserEmail); + SetSignFormField("identifier", kFakeUserEmail); ExecuteJsInSigninFrame("document.getElementById('nextButton').click();"); JsExpect("$('close-button-item').hidden"); @@ -39,21 +69,21 @@ IN_PROC_BROWSER_TEST_F(WebviewLoginTest, Basic) { chrome::NOTIFICATION_SESSION_STARTED, content::NotificationService::AllSources()); - SetSignFormField("password", OobeBaseTest::kFakeUserPassword); + SetSignFormField("password", kFakeUserPassword); ExecuteJsInSigninFrame("document.getElementById('nextButton').click();"); session_start_waiter.Wait(); } IN_PROC_BROWSER_TEST_F(WebviewLoginTest, BackButton) { - WaitForGaiaPageLoad(); + WaitForGaiaPageLoaded(); // Start: no back button, first page. JsExpect("$('back-button-item').hidden"); JsExpect("$('signin-frame').src.indexOf('#identifier') != -1"); // Next step: back button active, second page. - SetSignFormField("identifier", OobeBaseTest::kFakeUserEmail); + SetSignFormField("identifier", kFakeUserEmail); ExecuteJsInSigninFrame("document.getElementById('nextButton').click();"); JsExpect("!$('back-button-item').hidden"); JsExpect("$('signin-frame').src.indexOf('#challengepassword') != -1"); @@ -73,7 +103,7 @@ IN_PROC_BROWSER_TEST_F(WebviewLoginTest, BackButton) { chrome::NOTIFICATION_SESSION_STARTED, content::NotificationService::AllSources()); - SetSignFormField("password", OobeBaseTest::kFakeUserPassword); + SetSignFormField("password", kFakeUserPassword); ExecuteJsInSigninFrame("document.getElementById('nextButton').click();"); session_start_waiter.Wait(); diff --git a/chrome/browser/chromeos/login/wizard_controller_browsertest.cc b/chrome/browser/chromeos/login/wizard_controller_browsertest.cc index 3b71746..658a78a 100644 --- a/chrome/browser/chromeos/login/wizard_controller_browsertest.cc +++ b/chrome/browser/chromeos/login/wizard_controller_browsertest.cc @@ -7,8 +7,6 @@ #include "base/basictypes.h" #include "base/command_line.h" #include "base/compiler_specific.h" -#include "base/json/json_file_value_serializer.h" -#include "base/path_service.h" #include "base/prefs/pref_registry_simple.h" #include "base/prefs/pref_service.h" #include "base/prefs/pref_service_factory.h" @@ -50,7 +48,6 @@ #include "chrome/browser/chromeos/profiles/profile_helper.h" #include "chrome/browser/ui/webui/chromeos/login/oobe_ui.h" #include "chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h" -#include "chrome/common/chrome_constants.h" #include "chrome/common/chrome_paths.h" #include "chrome/common/chrome_switches.h" #include "chrome/common/pref_names.h" @@ -968,11 +965,7 @@ IN_PROC_BROWSER_TEST_F(WizardControllerBrokenLocalStateTest, ASSERT_EQ(1, fake_session_manager_client()->start_device_wipe_call_count()); } -// Boolean parameter is used to run this test for webview (true) and for -// iframe (false) GAIA sign in. -class WizardControllerProxyAuthOnSigninTest - : public WizardControllerTest, - public testing::WithParamInterface<bool> { +class WizardControllerProxyAuthOnSigninTest : public WizardControllerTest { protected: WizardControllerProxyAuthOnSigninTest() : proxy_server_(net::SpawnedTestServer::TYPE_BASIC_AUTH_PROXY, @@ -998,32 +991,6 @@ class WizardControllerProxyAuthOnSigninTest proxy_server_.host_port_pair().ToString()); } - bool SetUpUserDataDirectory() override { - base::FilePath user_data_dir; - CHECK(PathService::Get(chrome::DIR_USER_DATA, &user_data_dir)); - base::FilePath local_state_path = - user_data_dir.Append(chrome::kLocalStateFilename); - - // Set webview disabled flag only when local state file does not exist. - // Otherwise, we break PRE tests that leave state in it. - if (!base::PathExists(local_state_path)) { - base::DictionaryValue local_state_dict; - - if (!GetParam()) - local_state_dict.SetBoolean(prefs::kWebviewSigninDisabled, true); - - // TODO(paulmeyer): Re-enable webview version of this test - // (drop this condition) once http://crbug.com/452452 is fixed. - if (GetParam()) - local_state_dict.SetBoolean(prefs::kWebviewSigninDisabled, true); - - CHECK(JSONFileValueSerializer(local_state_path) - .Serialize(local_state_dict)); - } - - return WizardControllerTest::SetUpUserDataDirectory(); - } - net::SpawnedTestServer& proxy_server() { return proxy_server_; } private: @@ -1032,7 +999,7 @@ class WizardControllerProxyAuthOnSigninTest DISALLOW_COPY_AND_ASSIGN(WizardControllerProxyAuthOnSigninTest); }; -IN_PROC_BROWSER_TEST_P(WizardControllerProxyAuthOnSigninTest, +IN_PROC_BROWSER_TEST_F(WizardControllerProxyAuthOnSigninTest, ProxyAuthDialogOnSigninScreen) { content::WindowedNotificationObserver auth_needed_waiter( chrome::NOTIFICATION_AUTH_NEEDED, @@ -1044,10 +1011,6 @@ IN_PROC_BROWSER_TEST_P(WizardControllerProxyAuthOnSigninTest, auth_needed_waiter.Wait(); } -INSTANTIATE_TEST_CASE_P(WizardControllerProxyAuthOnSigninSuite, - WizardControllerProxyAuthOnSigninTest, - testing::Bool()); - class WizardControllerKioskFlowTest : public WizardControllerFlowTest { protected: WizardControllerKioskFlowTest() {} diff --git a/chrome/browser/chromeos/policy/blocking_login_browsertest.cc b/chrome/browser/chromeos/policy/blocking_login_browsertest.cc index 03b0504..dabc1f7 100644 --- a/chrome/browser/chromeos/policy/blocking_login_browsertest.cc +++ b/chrome/browser/chromeos/policy/blocking_login_browsertest.cc @@ -12,7 +12,6 @@ #include "chrome/browser/browser_process.h" #include "chrome/browser/chrome_notification_types.h" #include "chrome/browser/chromeos/login/existing_user_controller.h" -#include "chrome/browser/chromeos/login/test/oobe_base_test.h" #include "chrome/browser/chromeos/login/ui/webui_login_display.h" #include "chrome/browser/chromeos/login/wizard_controller.h" #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h" @@ -28,6 +27,7 @@ #include "content/public/browser/notification_registrar.h" #include "content/public/browser/notification_service.h" #include "content/public/test/test_utils.h" +#include "google_apis/gaia/fake_gaia.h" #include "google_apis/gaia/gaia_switches.h" #include "google_apis/gaia/gaia_urls.h" #include "net/http/http_status_code.h" @@ -81,35 +81,52 @@ struct BlockingLoginTestParam { }; class BlockingLoginTest - : public OobeBaseTest, + : public InProcessBrowserTest, public content::NotificationObserver, public testing::WithParamInterface<BlockingLoginTestParam> { public: BlockingLoginTest() : profile_added_(NULL) {} void SetUpCommandLine(base::CommandLine* command_line) override { - OobeBaseTest::SetUpCommandLine(command_line); - + // Initialize the test server early, so that we can use its base url for + // the command line flags. + ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady()); + + // Use the login manager screens and the gaia auth extension. + command_line->AppendSwitch(switches::kLoginManager); + command_line->AppendSwitch(switches::kForceLoginManagerInTests); + command_line->AppendSwitchASCII(switches::kLoginProfile, "user"); command_line->AppendSwitchASCII(::switches::kAuthExtensionPath, "gaia_auth"); + + // Redirect requests to gaia and the policy server to the test server. + command_line->AppendSwitchASCII(::switches::kGaiaUrl, + embedded_test_server()->base_url().spec()); + command_line->AppendSwitchASCII(::switches::kLsoUrl, + embedded_test_server()->base_url().spec()); command_line->AppendSwitchASCII( policy::switches::kDeviceManagementUrl, embedded_test_server()->GetURL("/device_management").spec()); } void SetUpOnMainThread() override { + fake_gaia_.Initialize(); + + embedded_test_server()->RegisterRequestHandler( + base::Bind(&BlockingLoginTest::HandleRequest, base::Unretained(this))); + embedded_test_server()->RegisterRequestHandler( + base::Bind(&FakeGaia::HandleRequest, base::Unretained(&fake_gaia_))); + registrar_.Add(this, chrome::NOTIFICATION_PROFILE_ADDED, content::NotificationService::AllSources()); - - OobeBaseTest::SetUpOnMainThread(); } void TearDownOnMainThread() override { RunUntilIdle(); EXPECT_TRUE(responses_.empty()); STLDeleteElements(&responses_); - OobeBaseTest::TearDownOnMainThread(); + EXPECT_TRUE(embedded_test_server()->ShutdownAndWaitUntilComplete()); } void Observe(int type, @@ -228,14 +245,10 @@ class BlockingLoginTest } protected: - void RegisterAdditionalRequestHandlers() override { - embedded_test_server()->RegisterRequestHandler( - base::Bind(&BlockingLoginTest::HandleRequest, base::Unretained(this))); - } - Profile* profile_added_; private: + FakeGaia fake_gaia_; std::vector<net::test_server::HttpResponse*> responses_; content::NotificationRegistrar registrar_; diff --git a/chrome/browser/resources/extensions/extension_list.js b/chrome/browser/resources/extensions/extension_list.js index f6967bd..47dd710 100644 --- a/chrome/browser/resources/extensions/extension_list.js +++ b/chrome/browser/resources/extensions/extension_list.js @@ -285,11 +285,32 @@ cr.define('extensions', function() { this.extensions_ = extensions; this.showExtensionNodes_(); resolve(); + + // |onUpdateFinished_| should be called after the list becomes visible + // in extensions.js. |resolve| is async, so |onUpdateFinished_| + // shouldn't be called directly. + this.extensionsUpdated_.then(this.onUpdateFinished_.bind(this)); }.bind(this)); }.bind(this)); return this.extensionsUpdated_; }, + /** Updates elements that need to be visible in order to update properly. */ + onUpdateFinished_: function() { + assert(!this.hidden); + assert(!this.parentElement.hidden); + + this.updateFocusableElements(); + + var idToHighlight = this.getIdQueryParam_(); + if (idToHighlight && $(idToHighlight)) + this.scrollToNode_(idToHighlight); + + var idToOpenOptions = this.getOptionsQueryParam_(); + if (idToOpenOptions && $(idToOpenOptions)) + this.showEmbeddedExtensionOptions_(idToOpenOptions, true); + }, + /** @return {number} The number of extensions being displayed. */ getNumExtensions: function() { return this.extensions_.length; @@ -339,14 +360,6 @@ cr.define('extensions', function() { assertInstanceof(node, ExtensionFocusRow).destroy(); } } - - var idToHighlight = this.getIdQueryParam_(); - if (idToHighlight && $(idToHighlight)) - this.scrollToNode_(idToHighlight); - - var idToOpenOptions = this.getOptionsQueryParam_(); - if (idToOpenOptions && $(idToOpenOptions)) - this.showEmbeddedExtensionOptions_(idToOpenOptions, true); }, /** Updates each row's focusable elements without rebuilding the grid. */ diff --git a/chrome/browser/resources/extensions/extensions.js b/chrome/browser/resources/extensions/extensions.js index 211ecba..3eba3da 100644 --- a/chrome/browser/resources/extensions/extensions.js +++ b/chrome/browser/resources/extensions/extensions.js @@ -263,7 +263,6 @@ cr.define('extensions', function() { var hasExtensions = extensionList.getNumExtensions() != 0; $('no-extensions').hidden = hasExtensions; $('extension-list-wrapper').hidden = !hasExtensions; - $('extension-settings-list').updateFocusableElements(); }.bind(this)); }, diff --git a/chrome/browser/resources/settings/a11y_page/a11y_page_style.html b/chrome/browser/resources/settings/a11y_page/a11y_page.css index ff6c3bd..2277ed3 100644 --- a/chrome/browser/resources/settings/a11y_page/a11y_page_style.html +++ b/chrome/browser/resources/settings/a11y_page/a11y_page.css @@ -1,7 +1,7 @@ -<link rel="import" href="chrome://resources/polymer/polymer/polymer.html"> -<link rel="import" href="chrome://resources/polymer/core-style/core-style.html"> +/* Copyright 2015 The Chromium Authors. All rights reserved. + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. */ -<core-style id="a11yPageStyle"> .autoclick-delay-label { -webkit-margin-end: 0; -webkit-margin-start: 40px; @@ -21,4 +21,3 @@ .more-a11y-link { margin-bottom: 10px; } -</core-style> diff --git a/chrome/browser/resources/settings/a11y_page/a11y_page.html b/chrome/browser/resources/settings/a11y_page/a11y_page.html index 8076bbd..cf7a15a 100644 --- a/chrome/browser/resources/settings/a11y_page/a11y_page.html +++ b/chrome/browser/resources/settings/a11y_page/a11y_page.html @@ -5,12 +5,11 @@ <link rel="import" href="chrome://resources/cr_elements/cr_dropdown_menu/cr_dropdown_menu.html"> <link rel="import" href="chrome://md-settings/settings_page/settings_page_header.html"> <link rel="import" href="chrome://md-settings/checkbox/checkbox.html"> -<link rel="import" href="a11y_page_style.html"> <polymer-element name="cr-settings-a11y-page"> <template> <link rel="stylesheet" href="chrome://md-settings/settings_page/settings_page.css"> - <core-style ref="a11yPageStyle"></core-style> + <link rel="stylesheet" href="a11y_page.css"> <paper-shadow layout vertical cross-fade> <div class="more-a11y-link"> <a href="https://chrome.google.com/webstore/category/collection/accessibility" diff --git a/chrome/browser/resources/settings/checkbox/checkbox_style.html b/chrome/browser/resources/settings/checkbox/checkbox.css index 550eda4..0acbdbd 100644 --- a/chrome/browser/resources/settings/checkbox/checkbox_style.html +++ b/chrome/browser/resources/settings/checkbox/checkbox.css @@ -1,7 +1,7 @@ -<link rel="import" href="chrome://resources/polymer/polymer/polymer.html"> -<link rel="import" href="chrome://resources/polymer/core-style/core-style.html"> +/* Copyright 2015 The Chromium Authors. All rights reserved. + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. */ -<core-style id="checkboxStyle"> #checkbox { -webkit-margin-end: 10px; } @@ -17,4 +17,3 @@ core-label { -webkit-margin-start: 10px; color: rgba(0, 0, 0, .5); } -</core-style> diff --git a/chrome/browser/resources/settings/checkbox/checkbox.html b/chrome/browser/resources/settings/checkbox/checkbox.html index e4e884c..e660fdb 100644 --- a/chrome/browser/resources/settings/checkbox/checkbox.html +++ b/chrome/browser/resources/settings/checkbox/checkbox.html @@ -2,11 +2,10 @@ <link rel="import" href="chrome://resources/cr_elements/cr_checkbox/cr_checkbox.html"> <link rel="import" href="chrome://resources/cr_elements/cr_events/cr_events.html"> <link rel="import" href="chrome://md-settings/pref_tracker/pref_tracker.html"> -<link rel="import" href="checkbox_style.html"> <polymer-element name="cr-settings-checkbox"> <template> - <core-style ref="checkboxStyle"></core-style> + <link rel="stylesheet" href="checkbox.css"> <cr-events id="events"></cr-events> <cr-settings-pref-tracker pref="{{pref}}"></cr-settings-pref-tracker> diff --git a/chrome/browser/resources/settings/settings_resources.grd b/chrome/browser/resources/settings/settings_resources.grd index 5ceed83..1012730 100644 --- a/chrome/browser/resources/settings/settings_resources.grd +++ b/chrome/browser/resources/settings/settings_resources.grd @@ -20,8 +20,8 @@ type="chrome_html" flattenhtml="true" allowexternalscript="true" /> - <structure name="IDR_SETTINGS_A11Y_PAGE_STYLE_HTML" - file="a11y_page/a11y_page_style.html" + <structure name="IDR_SETTINGS_A11Y_PAGE_CSS" + file="a11y_page/a11y_page.css" type="chrome_html" /> <structure name="IDR_SETTINGS_CHECKBOX_HTML" file="checkbox/checkbox.html" @@ -29,8 +29,8 @@ <structure name="IDR_SETTINGS_CHECKBOX_JS" file="checkbox/checkbox.js" type="chrome_html" /> - <structure name="IDR_SETTINGS_CHECKBOX_STYLE_HTML" - file="checkbox/checkbox_style.html" + <structure name="IDR_SETTINGS_CHECKBOX_CSS" + file="checkbox/checkbox.css" type="chrome_html" /> <structure name="IDR_SETTINGS_CR_SETTINGS_DRAWER_STYLE_HTML" file="settings_drawer/settings_drawer_style.html" diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index 9d65d26..0657d6f 100644 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -1081,10 +1081,6 @@ 'browser/chromeos/login/screenshot_testing/screenshot_tester.h', 'browser/chromeos/login/screenshot_testing/screenshot_testing_mixin.cc', 'browser/chromeos/login/screenshot_testing/screenshot_testing_mixin.h', - 'browser/chromeos/login/test/https_forwarder.cc', - 'browser/chromeos/login/test/https_forwarder.h', - 'browser/chromeos/login/test/oobe_base_test.cc', - 'browser/chromeos/login/test/oobe_base_test.h', 'browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc', 'browser/chromeos/login/users/wallpaper/wallpaper_manager_test_utils.cc', 'browser/chromeos/login/users/wallpaper/wallpaper_manager_test_utils.h', diff --git a/chrome/common/pref_names.cc b/chrome/common/pref_names.cc index c3678fc..2966dbf 100644 --- a/chrome/common/pref_names.cc +++ b/chrome/common/pref_names.cc @@ -1919,8 +1919,9 @@ const char kConsumerManagementStage[] = "consumer_management.stage"; const char kNewOobe[] = "NewOobe"; // A boolean pref. If set to true, experimental webview based signin flow -// is deactivated. -const char kWebviewSigninDisabled[] = "webview_signin_disabled"; +// activated. +const char kWebviewSigninEnabled[] = + "webview_signin_enabled"; #endif // defined(OS_CHROMEOS) // Whether there is a Flash version installed that supports clearing LSO data. diff --git a/chrome/common/pref_names.h b/chrome/common/pref_names.h index 3badee1..b40b4f0 100644 --- a/chrome/common/pref_names.h +++ b/chrome/common/pref_names.h @@ -677,7 +677,7 @@ extern const char kLogoutStartedLast[]; extern const char kConsumerManagementStage[]; extern const char kNewOobe[]; extern const char kConsumerManagementEnrollmentStage[]; -extern const char kWebviewSigninDisabled[]; +extern const char kWebviewSigninEnabled[]; #endif // defined(OS_CHROMEOS) extern const char kClearPluginLSODataEnabled[]; diff --git a/chrome/interactive_ui_tests.isolate b/chrome/interactive_ui_tests.isolate index 9823d0c..01c2675 100644 --- a/chrome/interactive_ui_tests.isolate +++ b/chrome/interactive_ui_tests.isolate @@ -130,7 +130,6 @@ '<(PRODUCT_DIR)/nacl_helper', '<(PRODUCT_DIR)/nacl_irt_x86_64.nexe', '<(PRODUCT_DIR)/resources/chromeos/', - 'browser/chromeos/login/test/https_forwarder.py', ], }, }], diff --git a/chrome/test/data/notifications/platform_notification_service.html b/chrome/test/data/notifications/platform_notification_service.html index d0b4124..7c4eb86 100644 --- a/chrome/test/data/notifications/platform_notification_service.html +++ b/chrome/test/data/notifications/platform_notification_service.html @@ -54,10 +54,7 @@ body: 'Contents', tag: 'replace-id', icon: 'icon.png', - silent: true, - data: [ - { property: 'value' } - ] + silent: true }); } diff --git a/chromecast/media/base/switching_media_renderer.cc b/chromecast/media/base/switching_media_renderer.cc index 79b7d1ef..1ca45b2 100644 --- a/chromecast/media/base/switching_media_renderer.cc +++ b/chromecast/media/base/switching_media_renderer.cc @@ -29,6 +29,7 @@ void SwitchingMediaRenderer::Initialize( const ::media::PipelineStatusCB& init_cb, const ::media::StatisticsCB& statistics_cb, const ::media::BufferingStateCB& buffering_state_cb, + const ::media::Renderer::PaintCB& paint_cb, const base::Closure& ended_cb, const ::media::PipelineStatusCB& error_cb, const base::Closure& waiting_for_decryption_key_cb) { @@ -52,7 +53,7 @@ void SwitchingMediaRenderer::Initialize( return GetRenderer()->Initialize( demuxer_stream_provider, init_cb, statistics_cb, buffering_state_cb, - ended_cb, error_cb, waiting_for_decryption_key_cb); + paint_cb, ended_cb, error_cb, waiting_for_decryption_key_cb); } ::media::Renderer* SwitchingMediaRenderer::GetRenderer() const { diff --git a/chromecast/media/base/switching_media_renderer.h b/chromecast/media/base/switching_media_renderer.h index 2739f40..830cef6 100644 --- a/chromecast/media/base/switching_media_renderer.h +++ b/chromecast/media/base/switching_media_renderer.h @@ -36,6 +36,7 @@ class SwitchingMediaRenderer : public ::media::Renderer { const ::media::PipelineStatusCB& init_cb, const ::media::StatisticsCB& statistics_cb, const ::media::BufferingStateCB& buffering_state_cb, + const ::media::Renderer::PaintCB& paint_cb, const base::Closure& ended_cb, const ::media::PipelineStatusCB& error_cb, const base::Closure& waiting_for_decryption_key_cb) override; diff --git a/chromecast/media/cma/filters/cma_renderer.cc b/chromecast/media/cma/filters/cma_renderer.cc index bc29b4f..1473f02 100644 --- a/chromecast/media/cma/filters/cma_renderer.cc +++ b/chromecast/media/cma/filters/cma_renderer.cc @@ -22,7 +22,6 @@ #include "media/base/pipeline_status.h" #include "media/base/time_delta_interpolator.h" #include "media/base/video_frame.h" -#include "media/base/video_renderer_sink.h" #include "ui/gfx/geometry/size.h" namespace chromecast { @@ -37,14 +36,12 @@ const base::TimeDelta kMaxDeltaFetcher( } // namespace -CmaRenderer::CmaRenderer(scoped_ptr<MediaPipeline> media_pipeline, - ::media::VideoRendererSink* video_renderer_sink) +CmaRenderer::CmaRenderer(scoped_ptr<MediaPipeline> media_pipeline) : media_task_runner_factory_( new BalancedMediaTaskRunnerFactory(kMaxDeltaFetcher)), media_pipeline_(media_pipeline.Pass()), audio_pipeline_(media_pipeline_->GetAudioPipeline()), video_pipeline_(media_pipeline_->GetVideoPipeline()), - video_renderer_sink_(video_renderer_sink), state_(kUninitialized), is_pending_transition_(false), has_audio_(false), @@ -79,6 +76,7 @@ void CmaRenderer::Initialize( const ::media::PipelineStatusCB& init_cb, const ::media::StatisticsCB& statistics_cb, const ::media::BufferingStateCB& buffering_state_cb, + const PaintCB& paint_cb, const base::Closure& ended_cb, const ::media::PipelineStatusCB& error_cb, const base::Closure& waiting_for_decryption_key_cb) { @@ -99,6 +97,7 @@ void CmaRenderer::Initialize( demuxer_stream_provider_ = demuxer_stream_provider; statistics_cb_ = statistics_cb; buffering_state_cb_ = buffering_state_cb; + paint_cb_ = paint_cb; ended_cb_ = ended_cb; error_cb_ = error_cb; // TODO(erickung): wire up waiting_for_decryption_key_cb. @@ -161,8 +160,7 @@ void CmaRenderer::StartPlayingFrom(base::TimeDelta time) { // Pipeline::StateTransitionTask). if (!initial_video_hole_created_) { initial_video_hole_created_ = true; - video_renderer_sink_->PaintFrameUsingOldRenderingPath( - ::media::VideoFrame::CreateHoleFrame(initial_natural_size_)); + paint_cb_.Run(::media::VideoFrame::CreateHoleFrame(initial_natural_size_)); } #endif @@ -385,8 +383,7 @@ void CmaRenderer::OnStatisticsUpdated( void CmaRenderer::OnNaturalSizeChanged(const gfx::Size& size) { DCHECK(thread_checker_.CalledOnValidThread()); #if defined(VIDEO_HOLE) - video_renderer_sink_->PaintFrameUsingOldRenderingPath( - ::media::VideoFrame::CreateHoleFrame(size)); + paint_cb_.Run(::media::VideoFrame::CreateHoleFrame(size)); #endif } diff --git a/chromecast/media/cma/filters/cma_renderer.h b/chromecast/media/cma/filters/cma_renderer.h index 25fec41..62efd7c 100644 --- a/chromecast/media/cma/filters/cma_renderer.h +++ b/chromecast/media/cma/filters/cma_renderer.h @@ -23,7 +23,6 @@ namespace media { class DemuxerStreamProvider; class TimeDeltaInterpolator; class VideoFrame; -class VideoRendererSink; } namespace chromecast { @@ -35,8 +34,7 @@ class VideoPipeline; class CmaRenderer : public ::media::Renderer { public: - CmaRenderer(scoped_ptr<MediaPipeline> media_pipeline, - ::media::VideoRendererSink* video_renderer_sink); + explicit CmaRenderer(scoped_ptr<MediaPipeline> media_pipeline); ~CmaRenderer() override; // ::media::Renderer implementation: @@ -45,6 +43,7 @@ class CmaRenderer : public ::media::Renderer { const ::media::PipelineStatusCB& init_cb, const ::media::StatisticsCB& statistics_cb, const ::media::BufferingStateCB& buffering_state_cb, + const PaintCB& paint_cb, const base::Closure& ended_cb, const ::media::PipelineStatusCB& error_cb, const base::Closure& waiting_for_decryption_key_cb) override; @@ -99,11 +98,11 @@ class CmaRenderer : public ::media::Renderer { scoped_ptr<MediaPipeline> media_pipeline_; AudioPipeline* audio_pipeline_; VideoPipeline* video_pipeline_; - ::media::VideoRendererSink* video_renderer_sink_; ::media::DemuxerStreamProvider* demuxer_stream_provider_; // Set of callbacks. + PaintCB paint_cb_; ::media::PipelineStatusCB init_cb_; ::media::StatisticsCB statistics_cb_; base::Closure ended_cb_; diff --git a/chromecast/renderer/media/chromecast_media_renderer_factory.cc b/chromecast/renderer/media/chromecast_media_renderer_factory.cc index 62fe771..cdfab39 100644 --- a/chromecast/renderer/media/chromecast_media_renderer_factory.cc +++ b/chromecast/renderer/media/chromecast_media_renderer_factory.cc @@ -29,8 +29,7 @@ ChromecastMediaRendererFactory::~ChromecastMediaRendererFactory() { scoped_ptr<::media::Renderer> ChromecastMediaRendererFactory::CreateRenderer( const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner, - ::media::AudioRendererSink* audio_renderer_sink, - ::media::VideoRendererSink* video_renderer_sink) { + ::media::AudioRendererSink* audio_renderer_sink) { if (!default_render_factory_) { // Chromecast doesn't have input audio devices, so leave this uninitialized ::media::AudioParameters input_audio_params; @@ -62,11 +61,10 @@ scoped_ptr<::media::Renderer> ChromecastMediaRendererFactory::CreateRenderer( content::RenderThread::Get()->GetIOMessageLoopProxy(), cma_load_type)); scoped_ptr<CmaRenderer> cma_renderer( - new CmaRenderer(cma_media_pipeline.Pass(), video_renderer_sink)); + new CmaRenderer(cma_media_pipeline.Pass())); scoped_ptr<::media::Renderer> default_media_render( default_render_factory_->CreateRenderer(media_task_runner, - audio_renderer_sink, - video_renderer_sink)); + audio_renderer_sink)); scoped_ptr<SwitchingMediaRenderer> media_renderer(new SwitchingMediaRenderer( default_media_render.Pass(), cma_renderer.Pass())); return media_renderer.Pass(); diff --git a/chromecast/renderer/media/chromecast_media_renderer_factory.h b/chromecast/renderer/media/chromecast_media_renderer_factory.h index 8206094..f880ff6 100644 --- a/chromecast/renderer/media/chromecast_media_renderer_factory.h +++ b/chromecast/renderer/media/chromecast_media_renderer_factory.h @@ -27,8 +27,7 @@ class ChromecastMediaRendererFactory : public ::media::RendererFactory { // ::media::RendererFactory implementation. scoped_ptr<::media::Renderer> CreateRenderer( const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner, - ::media::AudioRendererSink* audio_renderer_sink, - ::media::VideoRendererSink* video_renderer_sink) final; + ::media::AudioRendererSink* audio_renderer_sink) final; private: int render_frame_id_; diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc index e29759a..9ee5108 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc @@ -11,14 +11,10 @@ #include "base/single_thread_task_runner.h" #include "base/strings/string_util.h" #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator.h" +#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h" #include "components/data_reduction_proxy/core/common/data_reduction_proxy_config_values.h" #include "components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.h" -#include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h" -#include "net/base/load_flags.h" -#include "net/http/http_network_layer.h" #include "net/proxy/proxy_server.h" -#include "net/url_request/url_fetcher.h" -#include "net/url_request/url_fetcher_delegate.h" #include "net/url_request/url_request_context.h" #include "net/url_request/url_request_context_getter.h" #include "net/url_request/url_request_status.h" @@ -50,82 +46,9 @@ void RecordNetworkChangeEvent(DataReductionProxyNetworkChangeEvent event) { namespace data_reduction_proxy { -// Checks if the secure proxy is allowed by the carrier by sending a probe. -class SecureProxyChecker : public net::URLFetcherDelegate { - public: - SecureProxyChecker(net::URLRequestContext* url_request_context, - scoped_refptr<base::SingleThreadTaskRunner> io_task_runner) - : io_task_runner_(io_task_runner) { - DCHECK(io_task_runner_->BelongsToCurrentThread()); - - url_request_context_.reset(new net::URLRequestContext()); - url_request_context_->CopyFrom(url_request_context); - - net::HttpNetworkSession::Params params_modified = - *(url_request_context_->GetNetworkSessionParams()); - params_modified.enable_quic = false; - params_modified.next_protos = net::NextProtosWithSpdyAndQuic(false, false); - - http_network_layer_.reset(new net::HttpNetworkLayer( - new net::HttpNetworkSession(params_modified))); - url_request_context_->set_http_transaction_factory( - http_network_layer_.get()); - - url_request_context_getter_ = new net::TrivialURLRequestContextGetter( - url_request_context_.get(), io_task_runner_); - } - - void OnURLFetchComplete(const net::URLFetcher* source) override { - DCHECK(io_task_runner_->BelongsToCurrentThread()); - DCHECK_EQ(source, fetcher_.get()); - net::URLRequestStatus status = source->GetStatus(); - - std::string response; - source->GetResponseAsString(&response); - - fetcher_callback_.Run(response, status); - } - - void CheckIfSecureProxyIsAllowed(const GURL& secure_proxy_check_url, - FetcherResponseCallback fetcher_callback) { - DCHECK(io_task_runner_->BelongsToCurrentThread()); - fetcher_.reset(net::URLFetcher::Create(secure_proxy_check_url, - net::URLFetcher::GET, this)); - fetcher_->SetLoadFlags(net::LOAD_DISABLE_CACHE | net::LOAD_BYPASS_PROXY); - fetcher_->SetRequestContext(url_request_context_getter_.get()); - // Configure max retries to be at most kMaxRetries times for 5xx errors. - static const int kMaxRetries = 5; - fetcher_->SetMaxRetriesOn5xx(kMaxRetries); - fetcher_->SetAutomaticallyRetryOnNetworkChanges(kMaxRetries); - // The secure proxy check should not be redirected. Since the secure proxy - // check will inevitably fail if it gets redirected somewhere else (e.g. by - // a captive portal), short circuit that by giving up on the secure proxy - // check if it gets redirected. - fetcher_->SetStopOnRedirect(true); - - fetcher_callback_ = fetcher_callback; - - fetcher_->Start(); - } - - ~SecureProxyChecker() override {} - - private: - scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_; - - scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_; - scoped_ptr<net::URLRequestContext> url_request_context_; - scoped_ptr<net::HttpNetworkLayer> http_network_layer_; - - // The URLFetcher being used for the secure proxy check. - scoped_ptr<net::URLFetcher> fetcher_; - FetcherResponseCallback fetcher_callback_; - - DISALLOW_COPY_AND_ASSIGN(SecureProxyChecker); -}; - DataReductionProxyConfig::DataReductionProxyConfig( scoped_refptr<base::SingleThreadTaskRunner> io_task_runner, + scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner, net::NetLog* net_log, scoped_ptr<DataReductionProxyConfigValues> config_values, DataReductionProxyConfigurator* configurator, @@ -137,11 +60,13 @@ DataReductionProxyConfig::DataReductionProxyConfig( alternative_enabled_by_user_(false), config_values_(config_values.Pass()), io_task_runner_(io_task_runner), + ui_task_runner_(ui_task_runner), net_log_(net_log), configurator_(configurator), event_store_(event_store), url_request_context_getter_(nullptr) { DCHECK(io_task_runner); + DCHECK(ui_task_runner); DCHECK(configurator); DCHECK(event_store); } @@ -150,15 +75,15 @@ DataReductionProxyConfig::~DataReductionProxyConfig() { net::NetworkChangeNotifier::RemoveIPAddressObserver(this); } +void DataReductionProxyConfig::SetDataReductionProxyService( + base::WeakPtr<DataReductionProxyService> data_reduction_proxy_service) { + data_reduction_proxy_service_ = data_reduction_proxy_service; +} + void DataReductionProxyConfig::InitializeOnIOThread( net::URLRequestContextGetter* url_request_context_getter) { DCHECK(url_request_context_getter); url_request_context_getter_ = url_request_context_getter; - - DCHECK(url_request_context_getter_->GetURLRequestContext()); - secure_proxy_checker_.reset(new SecureProxyChecker( - url_request_context_getter_->GetURLRequestContext(), io_task_runner_)); - if (!config_values_->allowed()) return; @@ -331,13 +256,9 @@ void DataReductionProxyConfig::SetProxyConfig( if (enabled && !(alternative_enabled && !config_values_->alternative_fallback_allowed())) { - // It is safe to use base::Unretained here, since it gets executed - // synchronously on the IO thread, and |this| outlives - // |secure_proxy_checker_|. - SecureProxyCheck( - config_values_->secure_proxy_check_url(), - base::Bind(&DataReductionProxyConfig::HandleSecureProxyCheckResponse, - base::Unretained(this))); + ui_task_runner_->PostTask( + FROM_HERE, base::Bind(&DataReductionProxyConfig::StartSecureProxyCheck, + base::Unretained(this))); } } @@ -401,7 +322,16 @@ void DataReductionProxyConfig::LogProxyState(bool enabled, void DataReductionProxyConfig::HandleSecureProxyCheckResponse( const std::string& response, const net::URLRequestStatus& status) { - DCHECK(io_task_runner_->BelongsToCurrentThread()); + DCHECK(ui_task_runner_->BelongsToCurrentThread()); + io_task_runner_->PostTask( + FROM_HERE, + base::Bind( + &DataReductionProxyConfig::HandleSecureProxyCheckResponseOnIOThread, + base::Unretained(this), response, status)); +} + +void DataReductionProxyConfig::HandleSecureProxyCheckResponseOnIOThread( + const std::string& response, const net::URLRequestStatus& status) { if (event_store_) { event_store_->EndSecureProxyCheck(bound_net_log_, status.error()); } @@ -464,13 +394,9 @@ void DataReductionProxyConfig::OnIPAddressChanged() { return; } - // It is safe to use base::Unretained here, since it gets executed - // synchronously on the IO thread, and |this| outlives - // |secure_proxy_checker_|. - SecureProxyCheck( - config_values_->secure_proxy_check_url(), - base::Bind(&DataReductionProxyConfig::HandleSecureProxyCheckResponse, - base::Unretained(this))); + ui_task_runner_->PostTask( + FROM_HERE, base::Bind(&DataReductionProxyConfig::StartSecureProxyCheck, + base::Unretained(this))); } } @@ -506,21 +432,21 @@ void DataReductionProxyConfig::RecordSecureProxyCheckFetchResult( SECURE_PROXY_CHECK_FETCH_RESULT_COUNT); } -void DataReductionProxyConfig::SecureProxyCheck( - const GURL& secure_proxy_check_url, - FetcherResponseCallback fetcher_callback) { - DCHECK(io_task_runner_->BelongsToCurrentThread()); +void DataReductionProxyConfig::StartSecureProxyCheck() { + DCHECK(ui_task_runner_->BelongsToCurrentThread()); bound_net_log_ = net::BoundNetLog::Make( net_log_, net::NetLog::SOURCE_DATA_REDUCTION_PROXY); + if (data_reduction_proxy_service_) { + if (event_store_) { + event_store_->BeginSecureProxyCheck( + bound_net_log_, config_values_->secure_proxy_check_url()); + } - if (event_store_) { - event_store_->BeginSecureProxyCheck( - bound_net_log_, config_values_->secure_proxy_check_url()); + data_reduction_proxy_service_->SecureProxyCheck( + config_values_->secure_proxy_check_url(), + base::Bind(&DataReductionProxyConfig::HandleSecureProxyCheckResponse, + base::Unretained(this))); } - - DCHECK(secure_proxy_checker_); - secure_proxy_checker_->CheckIfSecureProxyIsAllowed(secure_proxy_check_url, - fetcher_callback); } void DataReductionProxyConfig::GetNetworkList( diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h index 9acf691..770c032 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h @@ -5,9 +5,6 @@ #ifndef COMPONENTS_DATA_REDUCTION_PROXY_CORE_BROWSER_DATA_REDUCTION_PROXY_CONFIG_H_ #define COMPONENTS_DATA_REDUCTION_PROXY_CORE_BROWSER_DATA_REDUCTION_PROXY_CONFIG_H_ -#include <string> - -#include "base/callback.h" #include "base/gtest_prod_util.h" #include "base/macros.h" #include "base/memory/ref_counted.h" @@ -20,8 +17,6 @@ #include "net/proxy/proxy_config.h" #include "net/proxy/proxy_retry_info.h" -class GURL; - namespace base { class SingleThreadTaskRunner; class TimeDelta; @@ -30,7 +25,6 @@ class TimeDelta; namespace net { class HostPortPair; class NetLog; -class URLFetcher; class URLRequest; class URLRequestContextGetter; class URLRequestStatus; @@ -38,14 +32,10 @@ class URLRequestStatus; namespace data_reduction_proxy { -typedef base::Callback<void(const std::string&, const net::URLRequestStatus&)> - FetcherResponseCallback; - class DataReductionProxyConfigValues; class DataReductionProxyConfigurator; class DataReductionProxyEventStore; class DataReductionProxyService; -class SecureProxyChecker; struct DataReductionProxyTypeInfo; // Values of the UMA DataReductionProxy.ProbeURL histogram. @@ -85,12 +75,16 @@ class DataReductionProxyConfig // which this instance will own. DataReductionProxyConfig( scoped_refptr<base::SingleThreadTaskRunner> io_task_runner, + scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner, net::NetLog* net_log, scoped_ptr<DataReductionProxyConfigValues> config_values, DataReductionProxyConfigurator* configurator, DataReductionProxyEventStore* event_store); ~DataReductionProxyConfig() override; + void SetDataReductionProxyService( + base::WeakPtr<DataReductionProxyService> data_reduction_proxy_service); + // Performs initialization on the IO thread. void InitializeOnIOThread( net::URLRequestContextGetter* url_request_context_getter); @@ -221,16 +215,16 @@ class DataReductionProxyConfig bool restricted, bool at_startup); - // Requests the given |secure_proxy_check_url|. Upon completion, returns the - // results to the caller via the |fetcher_callback|. Virtualized for unit - // testing. - virtual void SecureProxyCheck(const GURL& secure_proxy_check_url, - FetcherResponseCallback fetcher_callback); + // Begins a secure proxy check to determine if the Data Reduction Proxy is + // permitted to use the HTTPS proxy servers. + void StartSecureProxyCheck(); // Parses the secure proxy check responses and appropriately configures the // Data Reduction Proxy rules. virtual void HandleSecureProxyCheckResponse( const std::string& response, const net::URLRequestStatus& status); + virtual void HandleSecureProxyCheckResponseOnIOThread( + const std::string& response, const net::URLRequestStatus& status); // Adds the default proxy bypass rules for the Data Reduction Proxy. void AddDefaultProxyBypassRules(); @@ -252,8 +246,6 @@ class DataReductionProxyConfig bool is_https, base::TimeDelta* min_retry_delay) const; - scoped_ptr<SecureProxyChecker> secure_proxy_checker_; - bool restricted_by_carrier_; bool disabled_on_vpn_; bool unreachable_; @@ -267,6 +259,10 @@ class DataReductionProxyConfig // IO thread. scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_; + // |ui_task_runner_| should be the task runner for running operations on the + // UI thread. + scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_; + // The caller must ensure that the |net_log_|, if set, outlives this instance. // It is used to create new instances of |bound_net_log_| on secure proxy // checks. |bound_net_log_| permits the correlation of the begin and end @@ -287,6 +283,11 @@ class DataReductionProxyConfig // Enforce usage on the IO thread. base::ThreadChecker thread_checker_; + // A weak pointer to a |DataReductionProxyService| to perform secure proxy + // checks. The weak pointer is required since the |DataReductionProxyService| + // is destroyed before this instance of the |DataReductionProxyConfig|. + base::WeakPtr<DataReductionProxyService> data_reduction_proxy_service_; + DISALLOW_COPY_AND_ASSIGN(DataReductionProxyConfig); }; diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc index dbbde52..f6e836c 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc @@ -39,6 +39,7 @@ TestDataReductionProxyConfig::TestDataReductionProxyConfig( DataReductionProxyConfigurator* configurator, DataReductionProxyEventStore* event_store) : DataReductionProxyConfig(task_runner, + task_runner, net_log, config_values.Pass(), configurator, diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h index 759b29c..fba88ee 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h @@ -116,9 +116,6 @@ class MockDataReductionProxyConfig : public TestDataReductionProxyConfig { bool(const net::URLRequest& request, const net::ProxyConfig& data_reduction_proxy_config, base::TimeDelta* min_retry_delay)); - MOCK_METHOD2(SecureProxyCheck, - void(const GURL& secure_proxy_check_url, - FetcherResponseCallback fetcher_callback)); // UpdateConfigurator should always call LogProxyState exactly once. void UpdateConfigurator(bool enabled, diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc index 7b61eed..44746c1 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc @@ -127,11 +127,13 @@ class DataReductionProxyConfigTest : public testing::Test { !config()->restricted_by_carrier_, request_succeeded && (response == "OK")), request_succeeded, 1); + MockDataReductionProxyService* service = + test_context_->mock_data_reduction_proxy_service(); TestResponder responder; responder.response = response; responder.status = net::URLRequestStatus(net::URLRequestStatus::SUCCESS, net::OK); - EXPECT_CALL(*config(), SecureProxyCheck(_, _)) + EXPECT_CALL(*service, SecureProxyCheck(_, _)) .Times(1) .WillRepeatedly(testing::WithArgs<1>( testing::Invoke(&responder, &TestResponder::ExecuteCallback))); @@ -159,8 +161,9 @@ class DataReductionProxyConfigTest : public testing::Test { scoped_ptr<DataReductionProxyParams> params) { params->EnableQuic(false); return make_scoped_ptr(new DataReductionProxyConfig( - test_context_->task_runner(), test_context_->net_log(), params.Pass(), - test_context_->configurator(), test_context_->event_store())); + test_context_->task_runner(), test_context_->task_runner(), + test_context_->net_log(), params.Pass(), test_context_->configurator(), + test_context_->event_store())); } MockDataReductionProxyConfig* config() { diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc index 9034bae..52340f7 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc @@ -58,12 +58,12 @@ DataReductionProxyIOData::DataReductionProxyIOData( DataReductionProxyMutableConfigValues::CreateFromParams(params.get()); raw_mutable_config = mutable_config.get(); config_.reset(new DataReductionProxyConfig( - io_task_runner_, net_log, mutable_config.Pass(), configurator_.get(), - event_store_.get())); + io_task_runner_, ui_task_runner_, net_log, mutable_config.Pass(), + configurator_.get(), event_store_.get())); } else { - config_.reset( - new DataReductionProxyConfig(io_task_runner_, net_log, params.Pass(), - configurator_.get(), event_store_.get())); + config_.reset(new DataReductionProxyConfig( + io_task_runner_, ui_task_runner_, net_log, params.Pass(), + configurator_.get(), event_store_.get())); } // It is safe to use base::Unretained here, since it gets executed @@ -113,6 +113,7 @@ void DataReductionProxyIOData::SetDataReductionProxyService( DCHECK(ui_task_runner_->BelongsToCurrentThread()); service_ = data_reduction_proxy_service; url_request_context_getter_ = service_->url_request_context_getter(); + config()->SetDataReductionProxyService(data_reduction_proxy_service); // Using base::Unretained is safe here, unless the browser is being shut down // before the Initialize task can be executed. The task is only created as // part of class initialization. diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc index 1b90a14..3c17aa2 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc @@ -11,6 +11,9 @@ #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h" #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_service_observer.h" #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h" +#include "net/base/load_flags.h" +#include "net/url_request/url_fetcher.h" +#include "net/url_request/url_request_status.h" namespace data_reduction_proxy { @@ -106,4 +109,47 @@ DataReductionProxyService::GetWeakPtr() { return weak_factory_.GetWeakPtr(); } +void DataReductionProxyService::OnURLFetchComplete( + const net::URLFetcher* source) { + DCHECK(source == fetcher_.get()); + net::URLRequestStatus status = source->GetStatus(); + + std::string response; + source->GetResponseAsString(&response); + + fetcher_callback_.Run(response, status); +} + +net::URLFetcher* DataReductionProxyService::GetURLFetcherForSecureProxyCheck( + const GURL& secure_proxy_check_url) { + net::URLFetcher* fetcher = net::URLFetcher::Create( + secure_proxy_check_url, net::URLFetcher::GET, this); + fetcher->SetLoadFlags(net::LOAD_DISABLE_CACHE | net::LOAD_BYPASS_PROXY); + DCHECK(url_request_context_getter_); + fetcher->SetRequestContext(url_request_context_getter_); + // Configure max retries to be at most kMaxRetries times for 5xx errors. + static const int kMaxRetries = 5; + fetcher->SetMaxRetriesOn5xx(kMaxRetries); + fetcher->SetAutomaticallyRetryOnNetworkChanges(kMaxRetries); + // The secure proxy check should not be redirected. Since the secure proxy + // check will inevitably fail if it gets redirected somewhere else (e.g. by a + // captive portal), short circuit that by giving up on the secure proxy check + // if it gets redirected. + fetcher->SetStopOnRedirect(true); + return fetcher; +} + +void DataReductionProxyService::SecureProxyCheck( + const GURL& secure_proxy_check_url, + FetcherResponseCallback fetcher_callback) { + DCHECK(CalledOnValidThread()); + net::URLFetcher* fetcher = + GetURLFetcherForSecureProxyCheck(secure_proxy_check_url); + if (!fetcher) + return; + fetcher_.reset(fetcher); + fetcher_callback_ = fetcher_callback; + fetcher_->Start(); +} + } // namespace data_reduction_proxy diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h b/components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h index ed39307..0f9aed7 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h @@ -14,6 +14,7 @@ #include "base/single_thread_task_runner.h" #include "base/threading/non_thread_safe.h" #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_metrics.h" +#include "net/url_request/url_fetcher_delegate.h" class GURL; class PrefService; @@ -24,11 +25,16 @@ class TimeDelta; } namespace net { +class URLFetcher; class URLRequestContextGetter; +class URLRequestStatus; } namespace data_reduction_proxy { +typedef base::Callback<void(const std::string&, const net::URLRequestStatus&)> + FetcherResponseCallback; + class DataReductionProxyCompressionStats; class DataReductionProxyIOData; class DataReductionProxyServiceObserver; @@ -36,7 +42,8 @@ class DataReductionProxySettings; // Contains and initializes all Data Reduction Proxy objects that have a // lifetime based on the UI thread. -class DataReductionProxyService : public base::NonThreadSafe { +class DataReductionProxyService : public base::NonThreadSafe, + public net::URLFetcherDelegate { public: // The caller must ensure that |settings| and |request_context| remain alive // for the lifetime of the |DataReductionProxyService| instance. This instance @@ -49,7 +56,7 @@ class DataReductionProxyService : public base::NonThreadSafe { net::URLRequestContextGetter* request_context_getter, scoped_refptr<base::SingleThreadTaskRunner> io_task_runner); - virtual ~DataReductionProxyService(); + ~DataReductionProxyService() override; // Sets the DataReductionProxyIOData weak pointer. void SetIOData(base::WeakPtr<DataReductionProxyIOData> io_data); @@ -60,6 +67,12 @@ class DataReductionProxyService : public base::NonThreadSafe { // final step in initialization. bool Initialized() const; + // Requests the given |secure_proxy_check_url|. Upon completion, returns the + // results to the caller via the |fetcher_callback|. Virtualized for unit + // testing. + virtual void SecureProxyCheck(const GURL& secure_proxy_check_url, + FetcherResponseCallback fetcher_callback); + // Constructs compression stats. This should not be called if a valid // compression stats is passed into the constructor. void EnableCompressionStatisticsLogging( @@ -101,9 +114,22 @@ class DataReductionProxyService : public base::NonThreadSafe { base::WeakPtr<DataReductionProxyService> GetWeakPtr(); + protected: + // Virtualized for testing. Returns a fetcher to check if it is permitted to + // use the secure proxy. + virtual net::URLFetcher* GetURLFetcherForSecureProxyCheck( + const GURL& secure_proxy_check_url); + private: + // net::URLFetcherDelegate: + void OnURLFetchComplete(const net::URLFetcher* source) override; + net::URLRequestContextGetter* url_request_context_getter_; + // The URLFetcher being used for the secure proxy check. + scoped_ptr<net::URLFetcher> fetcher_; + FetcherResponseCallback fetcher_callback_; + // Tracks compression statistics to be displayed to the user. scoped_ptr<DataReductionProxyCompressionStats> compression_stats_; diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc index 1d64a4c..0b404ad 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc @@ -194,18 +194,6 @@ TEST(DataReductionProxySettingsStandaloneTest, TestEndToEndSecureProxyCheck) { .SkipSettingsInitialization() .Build(); - // Enabling QUIC should have no effect since secure proxy should not - // use QUIC. If secure proxy check incorrectly uses QUIC, the tests will - // fail because Mock sockets do not speak QUIC. - scoped_ptr<net::HttpNetworkSession::Params> params( - new net::HttpNetworkSession::Params()); - params->use_alternate_protocols = true; - params->enable_quic = true; - params->origin_to_force_quic_on = net::HostPortPair::FromString( - TestDataReductionProxyParams::DefaultSecureProxyCheckURL()); - - context.set_http_network_session_params(params.Pass()); - context.set_net_log(drp_test_context->net_log()); net::MockClientSocketFactory mock_socket_factory; context.set_client_socket_factory(&mock_socket_factory); diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h b/components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h index 4993ffe..7b2add4 100644 --- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h +++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h @@ -156,6 +156,9 @@ class MockDataReductionProxyService : public DataReductionProxyService { scoped_refptr<base::SingleThreadTaskRunner> io_task_runner); ~MockDataReductionProxyService() override; + MOCK_METHOD2(SecureProxyCheck, + void(const GURL& secure_proxy_check_url, + FetcherResponseCallback fetcher_callback)); MOCK_METHOD3(SetProxyPrefs, void(bool enabled, bool alternative_enabled, bool at_startup)); }; diff --git a/content/browser/notifications/notification_database_data.proto b/content/browser/notifications/notification_database_data.proto index 1fb6808..8b02197 100644 --- a/content/browser/notifications/notification_database_data.proto +++ b/content/browser/notifications/notification_database_data.proto @@ -35,7 +35,6 @@ message NotificationDatabaseDataProto { optional string tag = 5; optional string icon = 6; optional bool silent = 7; - optional bytes data = 8; } optional NotificationData notification_data = 4; diff --git a/content/browser/notifications/notification_database_data_conversions.cc b/content/browser/notifications/notification_database_data_conversions.cc index d656f7d..d183dd4 100644 --- a/content/browser/notifications/notification_database_data_conversions.cc +++ b/content/browser/notifications/notification_database_data_conversions.cc @@ -40,11 +40,6 @@ bool DeserializeNotificationDatabaseData(const std::string& input, notification_data->icon = GURL(payload.icon()); notification_data->silent = payload.silent(); - if (payload.data().length()) { - notification_data->data.assign(payload.data().begin(), - payload.data().end()); - } - return true; } @@ -69,11 +64,6 @@ bool SerializeNotificationDatabaseData(const NotificationDatabaseData& input, payload->set_icon(notification_data.icon.spec()); payload->set_silent(notification_data.silent); - if (notification_data.data.size()) { - payload->set_data(¬ification_data.data.front(), - notification_data.data.size()); - } - NotificationDatabaseDataProto message; message.set_notification_id(input.notification_id); message.set_origin(input.origin.spec()); diff --git a/content/browser/notifications/notification_database_data_unittest.cc b/content/browser/notifications/notification_database_data_unittest.cc index f57bfa8..df147d0 100644 --- a/content/browser/notifications/notification_database_data_unittest.cc +++ b/content/browser/notifications/notification_database_data_unittest.cc @@ -19,12 +19,8 @@ const char kNotificationLang[] = "nl"; const char kNotificationBody[] = "Hello, world!"; const char kNotificationTag[] = "my_tag"; const char kNotificationIconUrl[] = "https://example.com/icon.png"; -const unsigned char kNotificationData[] = { 0xdf, 0xff, 0x0, 0x0, 0xff, 0xdf }; TEST(NotificationDatabaseDataTest, SerializeAndDeserializeData) { - std::vector<char> developer_data( - kNotificationData, kNotificationData + arraysize(kNotificationData)); - PlatformNotificationData notification_data; notification_data.title = base::ASCIIToUTF16(kNotificationTitle); notification_data.direction = @@ -34,7 +30,6 @@ TEST(NotificationDatabaseDataTest, SerializeAndDeserializeData) { notification_data.tag = kNotificationTag; notification_data.icon = GURL(kNotificationIconUrl); notification_data.silent = true; - notification_data.data = developer_data; NotificationDatabaseData database_data; database_data.notification_id = kNotificationId; @@ -69,10 +64,6 @@ TEST(NotificationDatabaseDataTest, SerializeAndDeserializeData) { EXPECT_EQ(notification_data.tag, copied_notification_data.tag); EXPECT_EQ(notification_data.icon, copied_notification_data.icon); EXPECT_EQ(notification_data.silent, copied_notification_data.silent); - - ASSERT_EQ(developer_data.size(), copied_notification_data.data.size()); - for (size_t i = 0; i < developer_data.size(); ++i) - EXPECT_EQ(developer_data[i], copied_notification_data.data[i]); } } // namespace content diff --git a/content/child/notifications/notification_data_conversions.cc b/content/child/notifications/notification_data_conversions.cc index 639e555..a1b8128 100644 --- a/content/child/notifications/notification_data_conversions.cc +++ b/content/child/notifications/notification_data_conversions.cc @@ -25,7 +25,6 @@ PlatformNotificationData ToPlatformNotificationData( platform_data.tag = base::UTF16ToUTF8(web_data.tag); platform_data.icon = GURL(web_data.icon.string()); platform_data.silent = web_data.silent; - platform_data.data.assign(web_data.data.begin(), web_data.data.end()); return platform_data; } @@ -44,7 +43,6 @@ WebNotificationData ToWebNotificationData( web_data.tag = blink::WebString::fromUTF8(platform_data.tag); web_data.icon = blink::WebURL(platform_data.icon); web_data.silent = platform_data.silent; - web_data.data = platform_data.data; return web_data; } diff --git a/content/child/notifications/notification_data_conversions_unittest.cc b/content/child/notifications/notification_data_conversions_unittest.cc index daded25..f6a1424 100644 --- a/content/child/notifications/notification_data_conversions_unittest.cc +++ b/content/child/notifications/notification_data_conversions_unittest.cc @@ -4,8 +4,6 @@ #include "content/child/notifications/notification_data_conversions.h" -#include <stdint.h> - #include "base/strings/utf_string_conversions.h" #include "content/public/common/platform_notification_data.h" #include "testing/gtest/include/gtest/gtest.h" @@ -14,18 +12,17 @@ #include "third_party/WebKit/public/platform/modules/notifications/WebNotificationData.h" namespace content { +namespace { const char kNotificationTitle[] = "My Notification"; const char kNotificationLang[] = "nl"; const char kNotificationBody[] = "Hello, world!"; const char kNotificationTag[] = "my_tag"; const char kNotificationIconUrl[] = "https://example.com/icon.png"; -const unsigned char kNotificationData[] = { 0xdf, 0xff, 0x0, 0x0, 0xff, 0xdf }; -TEST(NotificationDataConversionsTest, ToPlatformNotificationData) { - std::vector<char> developer_data( - kNotificationData, kNotificationData + arraysize(kNotificationData)); +} +TEST(NotificationDataConversionsTest, ToPlatformNotificationData) { blink::WebNotificationData web_data( blink::WebString::fromUTF8(kNotificationTitle), blink::WebNotificationData::DirectionLeftToRight, @@ -33,8 +30,7 @@ TEST(NotificationDataConversionsTest, ToPlatformNotificationData) { blink::WebString::fromUTF8(kNotificationBody), blink::WebString::fromUTF8(kNotificationTag), blink::WebURL(GURL(kNotificationIconUrl)), - true /* silent */, - blink::WebVector<char>(developer_data)); + true /* silent */); PlatformNotificationData platform_data = ToPlatformNotificationData(web_data); EXPECT_EQ(base::ASCIIToUTF16(kNotificationTitle), platform_data.title); @@ -45,10 +41,6 @@ TEST(NotificationDataConversionsTest, ToPlatformNotificationData) { EXPECT_EQ(kNotificationTag, platform_data.tag); EXPECT_EQ(kNotificationIconUrl, platform_data.icon.spec()); EXPECT_TRUE(platform_data.silent); - - ASSERT_EQ(developer_data.size(), platform_data.data.size()); - for (size_t i = 0; i < developer_data.size(); ++i) - EXPECT_EQ(developer_data[i], platform_data.data[i]); } TEST(NotificationDataConversionsTest, @@ -68,9 +60,6 @@ TEST(NotificationDataConversionsTest, } TEST(NotificationDataConversionsTest, ToWebNotificationData) { - std::vector<char> developer_data( - kNotificationData, kNotificationData + arraysize(kNotificationData)); - PlatformNotificationData platform_data; platform_data.title = base::ASCIIToUTF16(kNotificationTitle); platform_data.direction = @@ -80,7 +69,6 @@ TEST(NotificationDataConversionsTest, ToWebNotificationData) { platform_data.tag = kNotificationTag; platform_data.icon = GURL(kNotificationIconUrl); platform_data.silent = true; - platform_data.data = developer_data; blink::WebNotificationData web_data = ToWebNotificationData(platform_data); EXPECT_EQ(kNotificationTitle, web_data.title); @@ -91,10 +79,6 @@ TEST(NotificationDataConversionsTest, ToWebNotificationData) { EXPECT_EQ(kNotificationTag, web_data.tag); EXPECT_EQ(kNotificationIconUrl, web_data.icon.string()); EXPECT_TRUE(web_data.silent); - - ASSERT_EQ(developer_data.size(), web_data.data.size()); - for (size_t i = 0; i < developer_data.size(); ++i) - EXPECT_EQ(developer_data[i], web_data.data[i]); } TEST(NotificationDataConversionsTest, ToWebNotificationDataDirectionality) { diff --git a/content/child/notifications/notification_manager.cc b/content/child/notifications/notification_manager.cc index b442baf..96963df 100644 --- a/content/child/notifications/notification_manager.cc +++ b/content/child/notifications/notification_manager.cc @@ -4,10 +4,7 @@ #include "content/child/notifications/notification_manager.h" -#include <cmath> - #include "base/lazy_instance.h" -#include "base/metrics/histogram_macros.h" #include "base/strings/string_number_conversions.h" #include "base/strings/utf_string_conversions.h" #include "base/thread_task_runner_handle.h" @@ -98,24 +95,6 @@ void NotificationManager::showPersistent( service_worker_registration)->registration_id(); scoped_ptr<blink::WebNotificationShowCallbacks> owned_callbacks(callbacks); - - // Verify that the author-provided payload size does not exceed our limit. - // This is an implementation-defined limit to prevent abuse of notification - // data as a storage mechanism. A UMA histogram records the requested sizes, - // which enables us to track how much data authors are attempting to store. - // - // If the size exceeds this limit, reject the showNotification() promise. This - // is outside of the boundaries set by the specification, but it gives authors - // an indication that something has gone wrong. - size_t author_data_size = notification_data.data.size(); - UMA_HISTOGRAM_MEMORY_KB("Notifications.AuthorDataSizeKB", - static_cast<int>(ceil(author_data_size / 1024.0))); - - if (author_data_size > PlatformNotificationData::kMaximumDeveloperDataSize) { - owned_callbacks->onError(); - return; - } - if (notification_data.icon.isEmpty()) { DisplayPersistentNotification(origin, notification_data, diff --git a/content/common/platform_notification_messages.h b/content/common/platform_notification_messages.h index 7ad1e6b..a4ae706 100644 --- a/content/common/platform_notification_messages.h +++ b/content/common/platform_notification_messages.h @@ -43,7 +43,6 @@ IPC_STRUCT_TRAITS_BEGIN(content::PlatformNotificationData) IPC_STRUCT_TRAITS_MEMBER(tag) IPC_STRUCT_TRAITS_MEMBER(icon) IPC_STRUCT_TRAITS_MEMBER(silent) - IPC_STRUCT_TRAITS_MEMBER(data) IPC_STRUCT_TRAITS_END() // Messages sent from the browser to the renderer. diff --git a/content/public/common/platform_notification_data.cc b/content/public/common/platform_notification_data.cc index 387dee6..5b918692 100644 --- a/content/public/common/platform_notification_data.cc +++ b/content/public/common/platform_notification_data.cc @@ -6,7 +6,9 @@ namespace content { -PlatformNotificationData::PlatformNotificationData() {} +PlatformNotificationData::PlatformNotificationData() + : direction(NotificationDirectionLeftToRight), + silent(false) {} PlatformNotificationData::~PlatformNotificationData() {} diff --git a/content/public/common/platform_notification_data.h b/content/public/common/platform_notification_data.h index 5b608c0..f6c999d 100644 --- a/content/public/common/platform_notification_data.h +++ b/content/public/common/platform_notification_data.h @@ -6,7 +6,6 @@ #define CONTENT_PUBLIC_COMMON_PLATFORM_NOTIFICATION_DATA_H_ #include <string> -#include <vector> #include "base/strings/string16.h" #include "content/common/content_export.h" @@ -21,10 +20,6 @@ struct CONTENT_EXPORT PlatformNotificationData { PlatformNotificationData(); ~PlatformNotificationData(); - // The maximum size of developer-provided data to be stored in the |data| - // property of this structure. - static const size_t kMaximumDeveloperDataSize = 1024 * 1024; - enum NotificationDirection { NotificationDirectionLeftToRight, NotificationDirectionRightToLeft, @@ -36,7 +31,7 @@ struct CONTENT_EXPORT PlatformNotificationData { base::string16 title; // Hint to determine the directionality of the displayed notification. - NotificationDirection direction = NotificationDirectionLeftToRight; + NotificationDirection direction; // BCP 47 language tag describing the notification's contents. Optional. std::string lang; @@ -53,11 +48,7 @@ struct CONTENT_EXPORT PlatformNotificationData { // Whether default notification indicators (sound, vibration, light) should // be suppressed. - bool silent = false; - - // Developer-provided data associated with the notification, in the form of - // a serialized string. Must not exceed |kMaximumDeveloperDataSize| bytes. - std::vector<char> data; + bool silent; }; } // namespace content diff --git a/content/renderer/media/android/webmediaplayer_android.cc b/content/renderer/media/android/webmediaplayer_android.cc index 284123a..7779004 100644 --- a/content/renderer/media/android/webmediaplayer_android.cc +++ b/content/renderer/media/android/webmediaplayer_android.cc @@ -1272,12 +1272,6 @@ void WebMediaPlayerAndroid::SetCurrentFrameInternal( current_frame_ = video_frame; } -bool WebMediaPlayerAndroid::UpdateCurrentFrame(base::TimeTicks deadline_min, - base::TimeTicks deadline_max) { - NOTIMPLEMENTED(); - return false; -} - scoped_refptr<media::VideoFrame> WebMediaPlayerAndroid::GetCurrentFrame() { scoped_refptr<VideoFrame> video_frame; { @@ -1288,7 +1282,8 @@ scoped_refptr<media::VideoFrame> WebMediaPlayerAndroid::GetCurrentFrame() { return video_frame; } -void WebMediaPlayerAndroid::PutCurrentFrame() { +void WebMediaPlayerAndroid::PutCurrentFrame( + const scoped_refptr<media::VideoFrame>& frame) { } void WebMediaPlayerAndroid::ResetStreamTextureProxy() { diff --git a/content/renderer/media/android/webmediaplayer_android.h b/content/renderer/media/android/webmediaplayer_android.h index 5d5f653..c1e30101 100644 --- a/content/renderer/media/android/webmediaplayer_android.h +++ b/content/renderer/media/android/webmediaplayer_android.h @@ -185,10 +185,8 @@ class WebMediaPlayerAndroid : public blink::WebMediaPlayer, // compositor thread. void SetVideoFrameProviderClient( cc::VideoFrameProvider::Client* client) override; - bool UpdateCurrentFrame(base::TimeTicks deadline_min, - base::TimeTicks deadline_max) override; scoped_refptr<media::VideoFrame> GetCurrentFrame() override; - void PutCurrentFrame() override; + void PutCurrentFrame(const scoped_refptr<media::VideoFrame>& frame) override; // Media player callback handlers. void OnMediaMetadataChanged(const base::TimeDelta& duration, int width, diff --git a/content/renderer/media/webmediaplayer_ms.cc b/content/renderer/media/webmediaplayer_ms.cc index de6f33b..f76d616 100644 --- a/content/renderer/media/webmediaplayer_ms.cc +++ b/content/renderer/media/webmediaplayer_ms.cc @@ -456,14 +456,6 @@ void WebMediaPlayerMS::SetVideoFrameProviderClient( video_frame_provider_client_ = client; } -bool WebMediaPlayerMS::UpdateCurrentFrame(base::TimeTicks deadline_min, - base::TimeTicks deadline_max) { - // TODO(dalecurtis): This should make use of the deadline interval to ensure - // the painted frame is correct for the given interval. - NOTREACHED(); - return false; -} - scoped_refptr<media::VideoFrame> WebMediaPlayerMS::GetCurrentFrame() { DVLOG(3) << "WebMediaPlayerMS::GetCurrentFrame"; base::AutoLock auto_lock(current_frame_lock_); @@ -475,7 +467,8 @@ scoped_refptr<media::VideoFrame> WebMediaPlayerMS::GetCurrentFrame() { return current_frame_; } -void WebMediaPlayerMS::PutCurrentFrame() { +void WebMediaPlayerMS::PutCurrentFrame( + const scoped_refptr<media::VideoFrame>& frame) { DVLOG(3) << "WebMediaPlayerMS::PutCurrentFrame"; DCHECK(pending_repaint_); pending_repaint_ = false; diff --git a/content/renderer/media/webmediaplayer_ms.h b/content/renderer/media/webmediaplayer_ms.h index 7310fd6..da99ef1 100644 --- a/content/renderer/media/webmediaplayer_ms.h +++ b/content/renderer/media/webmediaplayer_ms.h @@ -134,10 +134,8 @@ class WebMediaPlayerMS // VideoFrameProvider implementation. void SetVideoFrameProviderClient( cc::VideoFrameProvider::Client* client) override; - bool UpdateCurrentFrame(base::TimeTicks deadline_min, - base::TimeTicks deadline_max) override; scoped_refptr<media::VideoFrame> GetCurrentFrame() override; - void PutCurrentFrame() override; + void PutCurrentFrame(const scoped_refptr<media::VideoFrame>& frame) override; private: // The callback for VideoFrameProvider to signal a new frame is available. diff --git a/media/BUILD.gn b/media/BUILD.gn index 9914df2..27ef3aa 100644 --- a/media/BUILD.gn +++ b/media/BUILD.gn @@ -792,3 +792,34 @@ if (media_use_ffmpeg) { ] } } + +if (use_x11) { + executable("player_x11") { + sources = [ + "tools/player_x11/data_source_logger.cc", + "tools/player_x11/data_source_logger.h", + "tools/player_x11/gl_video_renderer.cc", + "tools/player_x11/gl_video_renderer.h", + "tools/player_x11/player_x11.cc", + "tools/player_x11/x11_video_renderer.cc", + "tools/player_x11/x11_video_renderer.h", + ] + configs += [ + ":media_config", + "//build/config/linux:x11", + "//build/config/linux:xext", + + # TODO(ajwong): Why does xext get a separate thing in //build/config/linux:BUILD.gn + # "//build/config/linux:xrender", + ] + deps = [ + ":media", + ":shared_memory_support", + "//base", + "//tools/xdisplaycheck", + "//ui/gl", + "//ui/gfx", + "//ui/gfx/geometry", + ] + } +} diff --git a/media/base/mock_filters.h b/media/base/mock_filters.h index 37940e6..9cbe50c 100644 --- a/media/base/mock_filters.h +++ b/media/base/mock_filters.h @@ -121,16 +121,17 @@ class MockVideoRenderer : public VideoRenderer { virtual ~MockVideoRenderer(); // VideoRenderer implementation. - MOCK_METHOD9(Initialize, - void(DemuxerStream* stream, - const PipelineStatusCB& init_cb, - const SetDecryptorReadyCB& set_decryptor_ready_cb, - const StatisticsCB& statistics_cb, - const BufferingStateCB& buffering_state_cb, - const base::Closure& ended_cb, - const PipelineStatusCB& error_cb, - const WallClockTimeCB& wall_clock_time_cb, - const base::Closure& waiting_for_decryption_key_cb)); + MOCK_METHOD10(Initialize, + void(DemuxerStream* stream, + const PipelineStatusCB& init_cb, + const SetDecryptorReadyCB& set_decryptor_ready_cb, + const StatisticsCB& statistics_cb, + const BufferingStateCB& buffering_state_cb, + const PaintCB& paint_cb, + const base::Closure& ended_cb, + const PipelineStatusCB& error_cb, + const WallClockTimeCB& wall_clock_time_cb, + const base::Closure& waiting_for_decryption_key_cb)); MOCK_METHOD1(Flush, void(const base::Closure& callback)); MOCK_METHOD1(StartPlayingFrom, void(base::TimeDelta)); MOCK_METHOD1(OnTimeStateChanged, void(bool)); @@ -169,11 +170,12 @@ class MockRenderer : public Renderer { virtual ~MockRenderer(); // Renderer implementation. - MOCK_METHOD7(Initialize, + MOCK_METHOD8(Initialize, void(DemuxerStreamProvider* demuxer_stream_provider, const PipelineStatusCB& init_cb, const StatisticsCB& statistics_cb, const BufferingStateCB& buffering_state_cb, + const PaintCB& paint_cb, const base::Closure& ended_cb, const PipelineStatusCB& error_cb, const base::Closure& waiting_for_decryption_key_cb)); diff --git a/media/base/pipeline.cc b/media/base/pipeline.cc index c39ddcd..aaa6cb6 100644 --- a/media/base/pipeline.cc +++ b/media/base/pipeline.cc @@ -68,6 +68,7 @@ void Pipeline::Start(Demuxer* demuxer, const PipelineStatusCB& seek_cb, const PipelineMetadataCB& metadata_cb, const BufferingStateCB& buffering_state_cb, + const PaintCB& paint_cb, const base::Closure& duration_change_cb, const AddTextTrackCB& add_text_track_cb, const base::Closure& waiting_for_decryption_key_cb) { @@ -76,6 +77,7 @@ void Pipeline::Start(Demuxer* demuxer, DCHECK(!seek_cb.is_null()); DCHECK(!metadata_cb.is_null()); DCHECK(!buffering_state_cb.is_null()); + DCHECK(!paint_cb.is_null()); base::AutoLock auto_lock(lock_); CHECK(!running_) << "Media pipeline is already running"; @@ -88,6 +90,7 @@ void Pipeline::Start(Demuxer* demuxer, seek_cb_ = seek_cb; metadata_cb_ = metadata_cb; buffering_state_cb_ = buffering_state_cb; + paint_cb_ = paint_cb; duration_change_cb_ = duration_change_cb; add_text_track_cb_ = add_text_track_cb; waiting_for_decryption_key_cb_ = waiting_for_decryption_key_cb; @@ -711,6 +714,7 @@ void Pipeline::InitializeRenderer(const PipelineStatusCB& done_cb) { done_cb, base::Bind(&Pipeline::OnUpdateStatistics, weak_this), base::Bind(&Pipeline::BufferingStateChanged, weak_this), + base::ResetAndReturn(&paint_cb_), base::Bind(&Pipeline::OnRendererEnded, weak_this), base::Bind(&Pipeline::OnError, weak_this), waiting_for_decryption_key_cb_); diff --git a/media/base/pipeline.h b/media/base/pipeline.h index f10bc39..30707b3 100644 --- a/media/base/pipeline.h +++ b/media/base/pipeline.h @@ -100,6 +100,8 @@ class MEDIA_EXPORT Pipeline : public DemuxerHost { // video in supported formats are known. // |buffering_state_cb| will be executed whenever there are changes in the // overall buffering state of the pipeline. + // |paint_cb| will be executed whenever there is a VideoFrame to be painted. + // It's safe to call this callback from any thread. // |duration_change_cb| optional callback that will be executed whenever the // presentation duration changes. // |add_text_track_cb| will be executed whenever a text track is added. @@ -113,6 +115,7 @@ class MEDIA_EXPORT Pipeline : public DemuxerHost { const PipelineStatusCB& seek_cb, const PipelineMetadataCB& metadata_cb, const BufferingStateCB& buffering_state_cb, + const PaintCB& paint_cb, const base::Closure& duration_change_cb, const AddTextTrackCB& add_text_track_cb, const base::Closure& waiting_for_decryption_key_cb); @@ -357,6 +360,7 @@ class MEDIA_EXPORT Pipeline : public DemuxerHost { PipelineStatusCB error_cb_; PipelineMetadataCB metadata_cb_; BufferingStateCB buffering_state_cb_; + PaintCB paint_cb_; base::Closure duration_change_cb_; AddTextTrackCB add_text_track_cb_; base::Closure waiting_for_decryption_key_cb_; diff --git a/media/base/pipeline_unittest.cc b/media/base/pipeline_unittest.cc index 09d9fdd..093b51e 100644 --- a/media/base/pipeline_unittest.cc +++ b/media/base/pipeline_unittest.cc @@ -84,6 +84,7 @@ class PipelineTest : public ::testing::Test { MOCK_METHOD1(OnError, void(PipelineStatus)); MOCK_METHOD1(OnMetadata, void(PipelineMetadata)); MOCK_METHOD1(OnBufferingStateChange, void(BufferingState)); + MOCK_METHOD1(OnVideoFramePaint, void(const scoped_refptr<VideoFrame>&)); MOCK_METHOD0(OnDurationChange, void()); private: @@ -169,9 +170,9 @@ class PipelineTest : public ::testing::Test { // Sets up expectations to allow the video renderer to initialize. void SetRendererExpectations() { - EXPECT_CALL(*renderer_, Initialize(_, _, _, _, _, _, _)) + EXPECT_CALL(*renderer_, Initialize(_, _, _, _, _, _, _, _)) .WillOnce(DoAll(SaveArg<3>(&buffering_state_cb_), - SaveArg<4>(&ended_cb_), + SaveArg<5>(&ended_cb_), PostCallback<1>(PIPELINE_OK))); EXPECT_CALL(*renderer_, HasAudio()).WillRepeatedly(Return(audio_stream())); EXPECT_CALL(*renderer_, HasVideo()).WillRepeatedly(Return(video_stream())); @@ -195,6 +196,8 @@ class PipelineTest : public ::testing::Test { base::Bind(&CallbackHelper::OnMetadata, base::Unretained(&callbacks_)), base::Bind(&CallbackHelper::OnBufferingStateChange, base::Unretained(&callbacks_)), + base::Bind(&CallbackHelper::OnVideoFramePaint, + base::Unretained(&callbacks_)), base::Bind(&CallbackHelper::OnDurationChange, base::Unretained(&callbacks_)), base::Bind(&PipelineTest::OnAddTextTrack, base::Unretained(this)), @@ -855,13 +858,13 @@ class PipelineTeardownTest : public PipelineTest { if (state == kInitRenderer) { if (stop_or_error == kStop) { - EXPECT_CALL(*renderer_, Initialize(_, _, _, _, _, _, _)) + EXPECT_CALL(*renderer_, Initialize(_, _, _, _, _, _, _, _)) .WillOnce(DoAll(Stop(pipeline_.get(), stop_cb), PostCallback<1>(PIPELINE_OK))); ExpectPipelineStopAndDestroyPipeline(); } else { status = PIPELINE_ERROR_INITIALIZATION_FAILED; - EXPECT_CALL(*renderer_, Initialize(_, _, _, _, _, _, _)) + EXPECT_CALL(*renderer_, Initialize(_, _, _, _, _, _, _, _)) .WillOnce(PostCallback<1>(status)); } @@ -870,7 +873,7 @@ class PipelineTeardownTest : public PipelineTest { return status; } - EXPECT_CALL(*renderer_, Initialize(_, _, _, _, _, _, _)) + EXPECT_CALL(*renderer_, Initialize(_, _, _, _, _, _, _, _)) .WillOnce(DoAll(SaveArg<3>(&buffering_state_cb_), PostCallback<1>(PIPELINE_OK))); diff --git a/media/base/renderer.h b/media/base/renderer.h index 31661c4..e3647f7 100644 --- a/media/base/renderer.h +++ b/media/base/renderer.h @@ -38,6 +38,9 @@ class MEDIA_EXPORT Renderer { // Permanent callbacks: // - |statistics_cb|: Executed periodically with rendering statistics. // - |buffering_state_cb|: Executed when buffering state is changed. + // - |paint_cb|: Executed when there is a VideoFrame ready to paint. Can be + // ignored if the Renderer handles the painting by itself. Can + // be called from any thread. // - |ended_cb|: Executed when rendering has reached the end of stream. // - |error_cb|: Executed if any error was encountered after initialization. // - |waiting_for_decryption_key_cb|: Executed whenever the key needed to @@ -47,6 +50,7 @@ class MEDIA_EXPORT Renderer { const PipelineStatusCB& init_cb, const StatisticsCB& statistics_cb, const BufferingStateCB& buffering_state_cb, + const PaintCB& paint_cb, const base::Closure& ended_cb, const PipelineStatusCB& error_cb, const base::Closure& waiting_for_decryption_key_cb) = 0; diff --git a/media/base/renderer_factory.h b/media/base/renderer_factory.h index aea3a53..211447e 100644 --- a/media/base/renderer_factory.h +++ b/media/base/renderer_factory.h @@ -18,7 +18,6 @@ class SingleThreadTaskRunner; namespace media { class AudioRendererSink; -class VideoRendererSink; // A factory class for creating media::Renderer to be used by media pipeline. class MEDIA_EXPORT RendererFactory { @@ -29,12 +28,10 @@ class MEDIA_EXPORT RendererFactory { // Creates and returns a Renderer. All methods of the created Renderer except // for GetMediaTime() will be called on the |media_task_runner|. // GetMediaTime() could be called on any thread. - // The created Renderer can use |audio_renderer_sink| to render audio and - // |video_renderer_sink| to render video. + // The created Renderer can use the |audio_renderer_sink| to render audio. virtual scoped_ptr<Renderer> CreateRenderer( const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner, - AudioRendererSink* audio_renderer_sink, - VideoRendererSink* video_renderer_sink) = 0; + AudioRendererSink* audio_renderer_sink) = 0; private: DISALLOW_COPY_AND_ASSIGN(RendererFactory); diff --git a/media/base/video_renderer.h b/media/base/video_renderer.h index 96a561f..46e842e 100644 --- a/media/base/video_renderer.h +++ b/media/base/video_renderer.h @@ -45,6 +45,9 @@ class MEDIA_EXPORT VideoRenderer { // |buffering_state_cb| is executed when video rendering has either run out of // data or has enough data to continue playback. // + // |paint_cb| is executed on the video frame timing thread whenever a new + // frame is available for painting. Can be called from any thread. + // // |ended_cb| is executed when video rendering has reached the end of stream. // // |error_cb| is executed if an error was encountered after initialization. @@ -60,6 +63,7 @@ class MEDIA_EXPORT VideoRenderer { const SetDecryptorReadyCB& set_decryptor_ready_cb, const StatisticsCB& statistics_cb, const BufferingStateCB& buffering_state_cb, + const PaintCB& paint_cb, const base::Closure& ended_cb, const PipelineStatusCB& error_cb, const WallClockTimeCB& wall_clock_time_cb, diff --git a/media/base/video_renderer_sink.h b/media/base/video_renderer_sink.h deleted file mode 100644 index a2852ff..0000000 --- a/media/base/video_renderer_sink.h +++ /dev/null @@ -1,64 +0,0 @@ -// Copyright 2015 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef MEDIA_BASE_VIDEO_RENDERER_SINK_H_ -#define MEDIA_BASE_VIDEO_RENDERER_SINK_H_ - -#include "base/basictypes.h" -#include "base/logging.h" -#include "base/memory/ref_counted.h" -#include "base/time/time.h" -#include "media/base/media_export.h" -#include "media/base/video_frame.h" - -namespace media { - -// VideoRendererSink is an interface representing the end-point for rendered -// video frames. An implementation is expected to periodically call Render() on -// a callback object. -class MEDIA_EXPORT VideoRendererSink { - public: - class RenderCallback { - public: - // Returns a VideoFrame for rendering which should be displayed within the - // presentation interval [|deadline_min|, |deadline_max|]. Returns NULL if - // no frame or no new frame (since the last Render() call) is available for - // rendering within the requested interval. Intervals are expected to be - // regular, contiguous, and monotonically increasing. Irregular intervals - // may affect the rendering decisions made by the underlying callback. - virtual scoped_refptr<VideoFrame> Render(base::TimeTicks deadline_min, - base::TimeTicks deadline_max) = 0; - - // Called by the sink when a VideoFrame previously returned via Render() was - // not actually rendered. Must be called before the next Render() call. - virtual void OnFrameDropped() = 0; - - virtual ~RenderCallback() {} - }; - - // Starts video rendering. See RenderCallback for more details. Must be - // called to receive Render() callbacks. Callbacks may start immediately, so - // |callback| must be ready to receive callbacks before calling Start(). - virtual void Start(RenderCallback* callback) = 0; - - // Stops video rendering, waits for any outstanding calls to the |callback| - // given to Start() to complete before returning. No new calls to |callback| - // will be issued after this method returns. May be used as a means of power - // conservation by the sink implementation, so clients should call this - // liberally if no new frames are expected. - virtual void Stop() = 0; - - // Instead of using a callback driven rendering path, allow clients to paint - // frames as they see fit without regard for the compositor. - // TODO(dalecurtis): This should be nuked once experiments show the new path - // is amazing and the old path is not! http://crbug.com/439548 - virtual void PaintFrameUsingOldRenderingPath( - const scoped_refptr<VideoFrame>& frame) = 0; - - virtual ~VideoRendererSink() {} -}; - -} // namespace media - -#endif // MEDIA_BASE_VIDEO_RENDERER_SINK_H_ diff --git a/media/base/wall_clock_time_source.cc b/media/base/wall_clock_time_source.cc index e58162e..d832546 100644 --- a/media/base/wall_clock_time_source.cc +++ b/media/base/wall_clock_time_source.cc @@ -68,7 +68,7 @@ base::TimeTicks WallClockTimeSource::GetWallClockTime(base::TimeDelta time) { // See notes about |time| values less than |base_time_| in TimeSource header. return reference_wall_ticks_ + base::TimeDelta::FromMicroseconds( - (time - base_time_).InMicroseconds() / playback_rate_); + (time - base_time_).InMicroseconds() * playback_rate_); } void WallClockTimeSource::SetTickClockForTesting( diff --git a/media/base/wall_clock_time_source_unittest.cc b/media/base/wall_clock_time_source_unittest.cc index fe8e04c..08a424d 100644 --- a/media/base/wall_clock_time_source_unittest.cc +++ b/media/base/wall_clock_time_source_unittest.cc @@ -13,7 +13,6 @@ class WallClockTimeSourceTest : public testing::Test { WallClockTimeSourceTest() : tick_clock_(new base::SimpleTestTickClock()) { time_source_.SetTickClockForTesting( scoped_ptr<base::TickClock>(tick_clock_)); - AdvanceTimeInSeconds(1); } ~WallClockTimeSourceTest() override {} @@ -29,18 +28,9 @@ class WallClockTimeSourceTest : public testing::Test { return time_source_.SetMediaTime(base::TimeDelta::FromSeconds(seconds)); } - bool IsWallClockNowForMediaTimeInSeconds(int seconds) { - return tick_clock_->NowTicks() == - time_source_.GetWallClockTime(base::TimeDelta::FromSeconds(seconds)); - } - - bool IsWallClockNullForMediaTimeInSeconds(int seconds) { - return time_source_.GetWallClockTime(base::TimeDelta::FromSeconds(seconds)) - .is_null(); - } - - protected: WallClockTimeSource time_source_; + + private: base::SimpleTestTickClock* tick_clock_; // Owned by |time_source_|. DISALLOW_COPY_AND_ASSIGN(WallClockTimeSourceTest); @@ -48,33 +38,26 @@ class WallClockTimeSourceTest : public testing::Test { TEST_F(WallClockTimeSourceTest, InitialTimeIsZero) { EXPECT_EQ(0, CurrentMediaTimeInSeconds()); - EXPECT_TRUE(IsWallClockNullForMediaTimeInSeconds(0)); } TEST_F(WallClockTimeSourceTest, InitialTimeIsNotTicking) { EXPECT_EQ(0, CurrentMediaTimeInSeconds()); - EXPECT_TRUE(IsWallClockNullForMediaTimeInSeconds(0)); AdvanceTimeInSeconds(100); EXPECT_EQ(0, CurrentMediaTimeInSeconds()); - EXPECT_TRUE(IsWallClockNullForMediaTimeInSeconds(0)); } TEST_F(WallClockTimeSourceTest, InitialPlaybackRateIsOne) { time_source_.StartTicking(); EXPECT_EQ(0, CurrentMediaTimeInSeconds()); - EXPECT_TRUE(IsWallClockNowForMediaTimeInSeconds(0)); AdvanceTimeInSeconds(100); EXPECT_EQ(100, CurrentMediaTimeInSeconds()); - EXPECT_TRUE(IsWallClockNowForMediaTimeInSeconds(100)); } TEST_F(WallClockTimeSourceTest, SetMediaTime) { EXPECT_EQ(0, CurrentMediaTimeInSeconds()); - EXPECT_TRUE(IsWallClockNullForMediaTimeInSeconds(0)); SetMediaTimeInSeconds(10); EXPECT_EQ(10, CurrentMediaTimeInSeconds()); - EXPECT_TRUE(IsWallClockNullForMediaTimeInSeconds(10)); } TEST_F(WallClockTimeSourceTest, SetPlaybackRate) { @@ -82,33 +65,26 @@ TEST_F(WallClockTimeSourceTest, SetPlaybackRate) { time_source_.SetPlaybackRate(0.5); EXPECT_EQ(0, CurrentMediaTimeInSeconds()); - EXPECT_TRUE(IsWallClockNowForMediaTimeInSeconds(0)); AdvanceTimeInSeconds(10); EXPECT_EQ(5, CurrentMediaTimeInSeconds()); - EXPECT_TRUE(IsWallClockNowForMediaTimeInSeconds(5)); time_source_.SetPlaybackRate(2); EXPECT_EQ(5, CurrentMediaTimeInSeconds()); - EXPECT_TRUE(IsWallClockNowForMediaTimeInSeconds(5)); AdvanceTimeInSeconds(10); EXPECT_EQ(25, CurrentMediaTimeInSeconds()); - EXPECT_TRUE(IsWallClockNowForMediaTimeInSeconds(25)); } TEST_F(WallClockTimeSourceTest, StopTicking) { time_source_.StartTicking(); EXPECT_EQ(0, CurrentMediaTimeInSeconds()); - EXPECT_TRUE(IsWallClockNowForMediaTimeInSeconds(0)); AdvanceTimeInSeconds(10); EXPECT_EQ(10, CurrentMediaTimeInSeconds()); - EXPECT_TRUE(IsWallClockNowForMediaTimeInSeconds(10)); time_source_.StopTicking(); AdvanceTimeInSeconds(10); EXPECT_EQ(10, CurrentMediaTimeInSeconds()); - EXPECT_TRUE(IsWallClockNullForMediaTimeInSeconds(10)); } } // namespace media diff --git a/media/blink/video_frame_compositor.cc b/media/blink/video_frame_compositor.cc index 90af145..447b6b1 100644 --- a/media/blink/video_frame_compositor.cc +++ b/media/blink/video_frame_compositor.cc @@ -4,8 +4,6 @@ #include "media/blink/video_frame_compositor.h" -#include "base/bind.h" -#include "base/message_loop/message_loop.h" #include "media/base/video_frame.h" namespace media { @@ -34,112 +32,35 @@ static bool IsOpaque(const scoped_refptr<VideoFrame>& frame) { } VideoFrameCompositor::VideoFrameCompositor( - const scoped_refptr<base::SingleThreadTaskRunner>& compositor_task_runner, const base::Callback<void(gfx::Size)>& natural_size_changed_cb, const base::Callback<void(bool)>& opacity_changed_cb) - : compositor_task_runner_(compositor_task_runner), - natural_size_changed_cb_(natural_size_changed_cb), + : natural_size_changed_cb_(natural_size_changed_cb), opacity_changed_cb_(opacity_changed_cb), - client_(nullptr), - rendering_(false), - callback_(nullptr) { + client_(NULL) { } VideoFrameCompositor::~VideoFrameCompositor() { - DCHECK(compositor_task_runner_->BelongsToCurrentThread()); - DCHECK(!callback_); - DCHECK(!rendering_); if (client_) client_->StopUsingProvider(); } -void VideoFrameCompositor::OnRendererStateUpdate() { - DCHECK(compositor_task_runner_->BelongsToCurrentThread()); - if (!client_) - return; - - base::AutoLock lock(lock_); - if (callback_) { - if (rendering_) - client_->StartRendering(); - - // TODO(dalecurtis): This will need to request the first frame so we have - // something to show, even if playback hasn't started yet. - } else if (rendering_) { - client_->StopRendering(); - } -} - -scoped_refptr<VideoFrame> -VideoFrameCompositor::GetCurrentFrameAndUpdateIfStale() { - // TODO(dalecurtis): Implement frame refresh when stale. - DCHECK(compositor_task_runner_->BelongsToCurrentThread()); - return GetCurrentFrame(); -} - void VideoFrameCompositor::SetVideoFrameProviderClient( cc::VideoFrameProvider::Client* client) { - DCHECK(compositor_task_runner_->BelongsToCurrentThread()); if (client_) client_->StopUsingProvider(); client_ = client; - OnRendererStateUpdate(); } scoped_refptr<VideoFrame> VideoFrameCompositor::GetCurrentFrame() { - DCHECK(compositor_task_runner_->BelongsToCurrentThread()); return current_frame_; } -void VideoFrameCompositor::PutCurrentFrame() { - DCHECK(compositor_task_runner_->BelongsToCurrentThread()); - // TODO(dalecurtis): Wire up a flag for RenderCallback::OnFrameDropped(). -} - -bool VideoFrameCompositor::UpdateCurrentFrame(base::TimeTicks deadline_min, - base::TimeTicks deadline_max) { - // TODO(dalecurtis): Wire this up to RenderCallback::Render(). - base::AutoLock lock(lock_); - return false; -} - -void VideoFrameCompositor::Start(RenderCallback* callback) { - NOTREACHED(); - - // Called from the media thread, so acquire the callback under lock before - // returning in case a Stop() call comes in before the PostTask is processed. - base::AutoLock lock(lock_); - callback_ = callback; - rendering_ = true; - compositor_task_runner_->PostTask( - FROM_HERE, base::Bind(&VideoFrameCompositor::OnRendererStateUpdate, - base::Unretained(this))); -} - -void VideoFrameCompositor::Stop() { - NOTREACHED(); - - // Called from the media thread, so release the callback under lock before - // returning to avoid a pending UpdateCurrentFrame() call occurring before - // the PostTask is processed. - base::AutoLock lock(lock_); - callback_ = nullptr; - rendering_ = false; - compositor_task_runner_->PostTask( - FROM_HERE, base::Bind(&VideoFrameCompositor::OnRendererStateUpdate, - base::Unretained(this))); +void VideoFrameCompositor::PutCurrentFrame( + const scoped_refptr<VideoFrame>& frame) { } -void VideoFrameCompositor::PaintFrameUsingOldRenderingPath( +void VideoFrameCompositor::UpdateCurrentFrame( const scoped_refptr<VideoFrame>& frame) { - if (!compositor_task_runner_->BelongsToCurrentThread()) { - compositor_task_runner_->PostTask( - FROM_HERE, - base::Bind(&VideoFrameCompositor::PaintFrameUsingOldRenderingPath, - base::Unretained(this), frame)); - return; - } - if (current_frame_.get() && current_frame_->natural_size() != frame->natural_size()) { natural_size_changed_cb_.Run(frame->natural_size()); diff --git a/media/blink/video_frame_compositor.h b/media/blink/video_frame_compositor.h index 02b6077..9e46663e 100644 --- a/media/blink/video_frame_compositor.h +++ b/media/blink/video_frame_compositor.h @@ -7,45 +7,24 @@ #include "base/callback.h" #include "base/memory/ref_counted.h" -#include "base/single_thread_task_runner.h" -#include "base/synchronization/lock.h" #include "cc/layers/video_frame_provider.h" #include "media/base/media_export.h" -#include "media/base/video_renderer_sink.h" #include "ui/gfx/geometry/size.h" namespace media { class VideoFrame; -// VideoFrameCompositor acts as a bridge between the media and cc layers for -// rendering video frames. I.e. a media::VideoRenderer will talk to this class -// from the media side, while a cc::VideoFrameProvider::Client will talk to it -// from the cc side. +// VideoFrameCompositor handles incoming frames by notifying the compositor and +// dispatching callbacks when detecting changes in video frames. // -// This class is responsible for requesting new frames from a video renderer in -// response to requests from the VFP::Client. Since the VFP::Client may stop -// issuing requests in response to visibility changes it is also responsible for -// ensuring the "freshness" of the current frame for programmatic frame -// requests; e.g., Canvas.drawImage() requests +// Typical usage is to deliver ready-to-be-displayed video frames to +// UpdateCurrentFrame() so that VideoFrameCompositor can take care of tracking +// changes in video frames and firing callbacks as needed. // -// This class is also responsible for detecting frames dropped by the compositor -// after rendering and signaling that information to a RenderCallback. It -// detects frames not dropped by verifying each GetCurrentFrame() is followed -// by a PutCurrentFrame() before the next UpdateCurrentFrame() call. -// -// VideoRenderSink::RenderCallback implementations must call Start() and Stop() -// once new frames are expected or are no longer expected to be ready; this data -// is relayed to the compositor to avoid extraneous callbacks. -// -// VideoFrameCompositor must live on the same thread as the compositor, though -// it may be constructed on any thread. +// VideoFrameCompositor must live on the same thread as the compositor. class MEDIA_EXPORT VideoFrameCompositor - : public VideoRendererSink, - NON_EXPORTED_BASE(public cc::VideoFrameProvider) { + : NON_EXPORTED_BASE(public cc::VideoFrameProvider) { public: - // |compositor_task_runner| is the task runner on which this class will live, - // though it may be constructed on any thread. - // // |natural_size_changed_cb| is run with the new natural size of the video // frame whenever a change in natural size is detected. It is not called the // first time UpdateCurrentFrame() is called. Run on the same thread as the @@ -55,61 +34,30 @@ class MEDIA_EXPORT VideoFrameCompositor // called the first time UpdateCurrentFrame() is called. Run on the same // thread as the caller of UpdateCurrentFrame(). // - // TODO(dalecurtis): Investigate the inconsistency between the callbacks with + // TODO(scherkus): Investigate the inconsistency between the callbacks with // respect to why we don't call |natural_size_changed_cb| on the first frame. // I suspect it was for historical reasons that no longer make sense. VideoFrameCompositor( - const scoped_refptr<base::SingleThreadTaskRunner>& compositor_task_runner, const base::Callback<void(gfx::Size)>& natural_size_changed_cb, const base::Callback<void(bool)>& opacity_changed_cb); - - // Destruction must happen on the compositor thread; Stop() must have been - // called before destruction starts. ~VideoFrameCompositor() override; - // Returns |current_frame_| if it was refreshed recently; otherwise, if - // |callback_| is available, requests a new frame and returns that one. - // - // This is required for programmatic frame requests where the compositor may - // have stopped issuing UpdateCurrentFrame() callbacks in response to - // visibility changes in the output layer. - scoped_refptr<VideoFrame> GetCurrentFrameAndUpdateIfStale(); - - // cc::VideoFrameProvider implementation. These methods must be called on the - // |compositor_task_runner_|. + // cc::VideoFrameProvider implementation. void SetVideoFrameProviderClient( cc::VideoFrameProvider::Client* client) override; - bool UpdateCurrentFrame(base::TimeTicks deadline_min, - base::TimeTicks deadline_max) override; scoped_refptr<VideoFrame> GetCurrentFrame() override; - void PutCurrentFrame() override; + void PutCurrentFrame(const scoped_refptr<VideoFrame>& frame) override; - // VideoRendererSink implementation. These methods must be called from the - // same thread (typically the media thread). - void Start(RenderCallback* callback) override; - void Stop() override; - void PaintFrameUsingOldRenderingPath( - const scoped_refptr<VideoFrame>& frame) override; + // Updates the current frame and notifies the compositor. + void UpdateCurrentFrame(const scoped_refptr<VideoFrame>& frame); private: - // Called on the compositor thread to start or stop rendering if rendering was - // previously started or stopped before we had a |callback_|. - void OnRendererStateUpdate(); - - scoped_refptr<base::SingleThreadTaskRunner> compositor_task_runner_; - - // These callbacks are executed on the compositor thread. base::Callback<void(gfx::Size)> natural_size_changed_cb_; base::Callback<void(bool)> opacity_changed_cb_; - // These values are only set and read on the compositor thread. cc::VideoFrameProvider::Client* client_; - scoped_refptr<VideoFrame> current_frame_; - // These values are updated and read from the media and compositor threads. - base::Lock lock_; - bool rendering_; - VideoRendererSink::RenderCallback* callback_; + scoped_refptr<VideoFrame> current_frame_; DISALLOW_COPY_AND_ASSIGN(VideoFrameCompositor); }; diff --git a/media/blink/video_frame_compositor_unittest.cc b/media/blink/video_frame_compositor_unittest.cc index b95a5000..43d750a 100644 --- a/media/blink/video_frame_compositor_unittest.cc +++ b/media/blink/video_frame_compositor_unittest.cc @@ -3,7 +3,6 @@ // found in the LICENSE file. #include "base/bind.h" -#include "base/message_loop/message_loop.h" #include "cc/layers/video_frame_provider.h" #include "media/base/video_frame.h" #include "media/blink/video_frame_compositor.h" @@ -16,7 +15,6 @@ class VideoFrameCompositorTest : public testing::Test, public: VideoFrameCompositorTest() : compositor_(new VideoFrameCompositor( - message_loop.task_runner(), base::Bind(&VideoFrameCompositorTest::NaturalSizeChanged, base::Unretained(this)), base::Bind(&VideoFrameCompositorTest::OpacityChanged, @@ -43,8 +41,6 @@ class VideoFrameCompositorTest : public testing::Test, private: // cc::VideoFrameProvider::Client implementation. void StopUsingProvider() override {} - void StartRendering() override {}; - void StopRendering() override {}; void DidReceiveFrame() override { ++did_receive_frame_count_; } @@ -60,7 +56,6 @@ class VideoFrameCompositorTest : public testing::Test, opaque_ = opaque; } - base::MessageLoop message_loop; scoped_ptr<VideoFrameCompositor> compositor_; int did_receive_frame_count_; int natural_size_changed_count_; @@ -75,12 +70,12 @@ TEST_F(VideoFrameCompositorTest, InitialValues) { EXPECT_FALSE(compositor()->GetCurrentFrame().get()); } -TEST_F(VideoFrameCompositorTest, PaintFrameUsingOldRenderingPath) { +TEST_F(VideoFrameCompositorTest, UpdateCurrentFrame) { scoped_refptr<VideoFrame> expected = VideoFrame::CreateEOSFrame(); // Should notify compositor synchronously. EXPECT_EQ(0, did_receive_frame_count()); - compositor()->PaintFrameUsingOldRenderingPath(expected); + compositor()->UpdateCurrentFrame(expected); scoped_refptr<VideoFrame> actual = compositor()->GetCurrentFrame(); EXPECT_EQ(expected, actual); EXPECT_EQ(1, did_receive_frame_count()); @@ -101,29 +96,29 @@ TEST_F(VideoFrameCompositorTest, NaturalSizeChanged) { EXPECT_EQ(0, natural_size_changed_count()); // Callback isn't fired for the first frame. - compositor()->PaintFrameUsingOldRenderingPath(initial_frame); + compositor()->UpdateCurrentFrame(initial_frame); EXPECT_EQ(0, natural_size().width()); EXPECT_EQ(0, natural_size().height()); EXPECT_EQ(0, natural_size_changed_count()); // Callback should be fired once. - compositor()->PaintFrameUsingOldRenderingPath(larger_frame); + compositor()->UpdateCurrentFrame(larger_frame); EXPECT_EQ(larger_size.width(), natural_size().width()); EXPECT_EQ(larger_size.height(), natural_size().height()); EXPECT_EQ(1, natural_size_changed_count()); - compositor()->PaintFrameUsingOldRenderingPath(larger_frame); + compositor()->UpdateCurrentFrame(larger_frame); EXPECT_EQ(larger_size.width(), natural_size().width()); EXPECT_EQ(larger_size.height(), natural_size().height()); EXPECT_EQ(1, natural_size_changed_count()); // Callback is fired once more when switching back to initial size. - compositor()->PaintFrameUsingOldRenderingPath(initial_frame); + compositor()->UpdateCurrentFrame(initial_frame); EXPECT_EQ(initial_size.width(), natural_size().width()); EXPECT_EQ(initial_size.height(), natural_size().height()); EXPECT_EQ(2, natural_size_changed_count()); - compositor()->PaintFrameUsingOldRenderingPath(initial_frame); + compositor()->UpdateCurrentFrame(initial_frame); EXPECT_EQ(initial_size.width(), natural_size().width()); EXPECT_EQ(initial_size, natural_size()); EXPECT_EQ(2, natural_size_changed_count()); @@ -142,22 +137,22 @@ TEST_F(VideoFrameCompositorTest, OpacityChanged) { EXPECT_EQ(0, opacity_changed_count()); // Callback is fired for the first frame. - compositor()->PaintFrameUsingOldRenderingPath(not_opaque_frame); + compositor()->UpdateCurrentFrame(not_opaque_frame); EXPECT_FALSE(opaque()); EXPECT_EQ(1, opacity_changed_count()); // Callback shouldn't be first subsequent times with same opaqueness. - compositor()->PaintFrameUsingOldRenderingPath(not_opaque_frame); + compositor()->UpdateCurrentFrame(not_opaque_frame); EXPECT_FALSE(opaque()); EXPECT_EQ(1, opacity_changed_count()); // Callback is fired when using opacity changes. - compositor()->PaintFrameUsingOldRenderingPath(opaque_frame); + compositor()->UpdateCurrentFrame(opaque_frame); EXPECT_TRUE(opaque()); EXPECT_EQ(2, opacity_changed_count()); // Callback shouldn't be first subsequent times with same opaqueness. - compositor()->PaintFrameUsingOldRenderingPath(opaque_frame); + compositor()->UpdateCurrentFrame(opaque_frame); EXPECT_TRUE(opaque()); EXPECT_EQ(2, opacity_changed_count()); } diff --git a/media/blink/webmediaplayer_impl.cc b/media/blink/webmediaplayer_impl.cc index 8fa7a7f..ab62772 100644 --- a/media/blink/webmediaplayer_impl.cc +++ b/media/blink/webmediaplayer_impl.cc @@ -133,13 +133,8 @@ WebMediaPlayerImpl::WebMediaPlayerImpl( context_3d_cb_(params.context_3d_cb()), supports_save_(true), chunk_demuxer_(NULL), - // Threaded compositing isn't enabled universally yet. - compositor_task_runner_( - params.compositor_task_runner() - ? params.compositor_task_runner() - : base::MessageLoop::current()->task_runner()), + compositor_task_runner_(params.compositor_task_runner()), compositor_(new VideoFrameCompositor( - compositor_task_runner_, BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnNaturalSizeChanged), BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnOpacityChanged))), encrypted_media_support_(cdm_factory, @@ -149,6 +144,10 @@ WebMediaPlayerImpl::WebMediaPlayerImpl( AsWeakPtr(), base::Bind(&IgnoreCdmAttached))), renderer_factory_(renderer_factory.Pass()) { + // Threaded compositing isn't enabled universally yet. + if (!compositor_task_runner_.get()) + compositor_task_runner_ = base::MessageLoopProxy::current(); + media_log_->AddEvent( media_log_->CreateEvent(MediaLogEvent::WEBMEDIAPLAYER_CREATED)); @@ -903,13 +902,14 @@ void WebMediaPlayerImpl::StartPipeline() { pipeline_.Start( demuxer_.get(), - renderer_factory_->CreateRenderer( - media_task_runner_, audio_source_provider_.get(), compositor_), + renderer_factory_->CreateRenderer(media_task_runner_, + audio_source_provider_.get()), BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnPipelineEnded), BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnPipelineError), BIND_TO_RENDER_LOOP1(&WebMediaPlayerImpl::OnPipelineSeeked, false), BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnPipelineMetadata), BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnPipelineBufferingStateChanged), + base::Bind(&WebMediaPlayerImpl::FrameReady, base::Unretained(this)), BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnDurationChanged), BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnAddTextTrack), BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnWaitingForDecryptionKey)); @@ -980,12 +980,21 @@ void WebMediaPlayerImpl::OnOpacityChanged(bool opaque) { video_weblayer_->setOpaque(opaque_); } +void WebMediaPlayerImpl::FrameReady( + const scoped_refptr<VideoFrame>& frame) { + compositor_task_runner_->PostTask( + FROM_HERE, + base::Bind(&VideoFrameCompositor::UpdateCurrentFrame, + base::Unretained(compositor_), + frame)); +} + static void GetCurrentFrameAndSignal( VideoFrameCompositor* compositor, scoped_refptr<VideoFrame>* video_frame_out, base::WaitableEvent* event) { TRACE_EVENT0("media", "GetCurrentFrameAndSignal"); - *video_frame_out = compositor->GetCurrentFrameAndUpdateIfStale(); + *video_frame_out = compositor->GetCurrentFrame(); event->Signal(); } diff --git a/media/media.gyp b/media/media.gyp index 272ee39..47f8df3 100644 --- a/media/media.gyp +++ b/media/media.gyp @@ -1667,6 +1667,44 @@ }, ], # targets }], + ['use_x11==1', { + 'targets': [ + { + 'target_name': 'player_x11', + 'type': 'executable', + 'dependencies': [ + 'media', + 'shared_memory_support', + '../base/base.gyp:base', + '../ui/gl/gl.gyp:gl', + '../ui/gfx/gfx.gyp:gfx', + '../ui/gfx/gfx.gyp:gfx_geometry', + '../build/linux/system.gyp:x11', + '../build/linux/system.gyp:xext', + '../build/linux/system.gyp:xrender', + ], + 'conditions': [ + # Linux/Solaris need libdl for dlopen() and friends. + ['OS=="linux" or OS=="solaris"', { + 'link_settings': { + 'libraries': [ + '-ldl', + ], + }, + }], + ], + 'sources': [ + 'tools/player_x11/data_source_logger.cc', + 'tools/player_x11/data_source_logger.h', + 'tools/player_x11/gl_video_renderer.cc', + 'tools/player_x11/gl_video_renderer.h', + 'tools/player_x11/player_x11.cc', + 'tools/player_x11/x11_video_renderer.cc', + 'tools/player_x11/x11_video_renderer.h', + ], + }, + ], + }], ['OS=="android"', { 'targets': [ { diff --git a/media/mojo/services/mojo_renderer_factory.cc b/media/mojo/services/mojo_renderer_factory.cc index 2833222..73ef9bf 100644 --- a/media/mojo/services/mojo_renderer_factory.cc +++ b/media/mojo/services/mojo_renderer_factory.cc @@ -21,8 +21,7 @@ MojoRendererFactory::~MojoRendererFactory() { scoped_ptr<Renderer> MojoRendererFactory::CreateRenderer( const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner, - AudioRendererSink* /* audio_renderer_sink */, - VideoRendererSink* /* video_renderer_sink */) { + AudioRendererSink* /* audio_renderer_sink */) { mojo::MediaRendererPtr mojo_media_renderer; service_provider_->ConnectToService(&mojo_media_renderer); return scoped_ptr<Renderer>( diff --git a/media/mojo/services/mojo_renderer_factory.h b/media/mojo/services/mojo_renderer_factory.h index 944fae5..2aa6cfc 100644 --- a/media/mojo/services/mojo_renderer_factory.h +++ b/media/mojo/services/mojo_renderer_factory.h @@ -33,8 +33,7 @@ class MEDIA_EXPORT MojoRendererFactory : public RendererFactory { scoped_ptr<Renderer> CreateRenderer( const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner, - AudioRendererSink* audio_renderer_sink, - VideoRendererSink* video_renderer_sink) final; + AudioRendererSink* audio_renderer_sink) final; private: scoped_ptr<ServiceProvider> service_provider_; diff --git a/media/mojo/services/mojo_renderer_impl.cc b/media/mojo/services/mojo_renderer_impl.cc index d1b41a6..e4a1332 100644 --- a/media/mojo/services/mojo_renderer_impl.cc +++ b/media/mojo/services/mojo_renderer_impl.cc @@ -33,12 +33,14 @@ MojoRendererImpl::~MojoRendererImpl() { // Connection to |remote_media_renderer_| will error-out here. } -// TODO(xhwang): Support |waiting_for_decryption_key_cb| if needed. +// TODO(xhwang): Support |paint_cb| and |waiting_for_decryption_key_cb|, +// if needed. void MojoRendererImpl::Initialize( DemuxerStreamProvider* demuxer_stream_provider, const PipelineStatusCB& init_cb, const StatisticsCB& statistics_cb, const BufferingStateCB& buffering_state_cb, + const PaintCB& /* paint_cb */, const base::Closure& ended_cb, const PipelineStatusCB& error_cb, const base::Closure& /* waiting_for_decryption_key_cb */) { diff --git a/media/mojo/services/mojo_renderer_impl.h b/media/mojo/services/mojo_renderer_impl.h index cea3284..338bcb8 100644 --- a/media/mojo/services/mojo_renderer_impl.h +++ b/media/mojo/services/mojo_renderer_impl.h @@ -37,6 +37,7 @@ class MojoRendererImpl : public Renderer, public mojo::MediaRendererClient { const PipelineStatusCB& init_cb, const StatisticsCB& statistics_cb, const BufferingStateCB& buffering_state_cb, + const PaintCB& paint_cb, const base::Closure& ended_cb, const PipelineStatusCB& error_cb, const base::Closure& waiting_for_decryption_key_cb) override; diff --git a/media/mojo/services/mojo_renderer_service.cc b/media/mojo/services/mojo_renderer_service.cc index 0601c2c..7b6884d 100644 --- a/media/mojo/services/mojo_renderer_service.cc +++ b/media/mojo/services/mojo_renderer_service.cc @@ -14,7 +14,6 @@ #include "media/base/decryptor.h" #include "media/base/media_log.h" #include "media/base/video_renderer.h" -#include "media/base/video_renderer_sink.h" #include "media/mojo/services/demuxer_stream_provider_shim.h" #include "media/mojo/services/renderer_config.h" #include "media/renderers/audio_renderer_impl.h" @@ -26,6 +25,9 @@ namespace media { // Time interval to update media time. const int kTimeUpdateIntervalMs = 50; +static void PaintNothing(const scoped_refptr<VideoFrame>& frame) { +} + MojoRendererService::MojoRendererService() : state_(STATE_UNINITIALIZED), last_media_time_usec_(0), @@ -38,7 +40,6 @@ MojoRendererService::MojoRendererService() scoped_refptr<MediaLog> media_log(new MediaLog()); RendererConfig* renderer_config = RendererConfig::Get(); audio_renderer_sink_ = renderer_config->GetAudioRendererSink(); - video_renderer_sink_ = renderer_config->GetVideoRendererSink(); scoped_ptr<AudioRenderer> audio_renderer(new AudioRendererImpl( task_runner, audio_renderer_sink_.get(), @@ -48,7 +49,7 @@ MojoRendererService::MojoRendererService() renderer_config->GetAudioHardwareConfig(), media_log)); scoped_ptr<VideoRenderer> video_renderer(new VideoRendererImpl( - task_runner, video_renderer_sink_.get(), + task_runner, renderer_config->GetVideoDecoders(task_runner, base::Bind(&MediaLog::AddLogEvent, media_log)).Pass(), @@ -112,6 +113,7 @@ void MojoRendererService::OnStreamReady(const mojo::Closure& callback) { &MojoRendererService::OnRendererInitializeDone, weak_this_, callback), base::Bind(&MojoRendererService::OnUpdateStatistics, weak_this_), base::Bind(&MojoRendererService::OnBufferingStateChanged, weak_this_), + base::Bind(&PaintNothing), base::Bind(&MojoRendererService::OnRendererEnded, weak_this_), base::Bind(&MojoRendererService::OnError, weak_this_), base::Bind(base::DoNothing)); diff --git a/media/mojo/services/mojo_renderer_service.h b/media/mojo/services/mojo_renderer_service.h index 6003515..2046e9d 100644 --- a/media/mojo/services/mojo_renderer_service.h +++ b/media/mojo/services/mojo_renderer_service.h @@ -27,7 +27,6 @@ namespace media { class AudioRendererSink; class DemuxerStreamProviderShim; class Renderer; -class VideoRendererSink; // A mojo::MediaRenderer implementation that uses media::AudioRenderer to // decode and render audio to a sink obtained from the ApplicationConnection. @@ -90,7 +89,6 @@ class MEDIA_EXPORT MojoRendererService State state_; scoped_refptr<AudioRendererSink> audio_renderer_sink_; - scoped_ptr<VideoRendererSink> video_renderer_sink_; scoped_ptr<Renderer> renderer_; scoped_ptr<DemuxerStreamProviderShim> stream_provider_; diff --git a/media/mojo/services/renderer_config.cc b/media/mojo/services/renderer_config.cc index 5522da2..311a260 100644 --- a/media/mojo/services/renderer_config.cc +++ b/media/mojo/services/renderer_config.cc @@ -34,10 +34,6 @@ scoped_refptr<AudioRendererSink> RendererConfig::GetAudioRendererSink() { return renderer_config_->GetAudioRendererSink(); } -scoped_ptr<VideoRendererSink> RendererConfig::GetVideoRendererSink() { - return renderer_config_->GetVideoRendererSink(); -} - const AudioHardwareConfig& RendererConfig::GetAudioHardwareConfig() { return renderer_config_->GetAudioHardwareConfig(); } diff --git a/media/mojo/services/renderer_config.h b/media/mojo/services/renderer_config.h index 4a531d5..08b7e0b 100644 --- a/media/mojo/services/renderer_config.h +++ b/media/mojo/services/renderer_config.h @@ -13,7 +13,6 @@ #include "media/base/audio_renderer_sink.h" #include "media/base/media_log.h" #include "media/base/video_decoder.h" -#include "media/base/video_renderer_sink.h" namespace media { @@ -35,13 +34,13 @@ class PlatformRendererConfig { const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner, const LogCB& media_log_cb) = 0; - // The output sink used for rendering audio or video respectively. + // The audio output sink used for rendering audio. virtual scoped_refptr<AudioRendererSink> GetAudioRendererSink() = 0; - virtual scoped_ptr<VideoRendererSink> GetVideoRendererSink() = 0; // The platform's audio hardware configuration. Note, this must remain // constant for the lifetime of the PlatformRendererConfig. virtual const AudioHardwareConfig& GetAudioHardwareConfig() = 0; + }; class RendererConfig { @@ -58,7 +57,6 @@ class RendererConfig { const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner, const LogCB& media_log_cb); scoped_refptr<AudioRendererSink> GetAudioRendererSink(); - scoped_ptr<VideoRendererSink> GetVideoRendererSink(); const AudioHardwareConfig& GetAudioHardwareConfig(); private: diff --git a/media/mojo/services/renderer_config_default.cc b/media/mojo/services/renderer_config_default.cc index 7cee1a7..66464a9 100644 --- a/media/mojo/services/renderer_config_default.cc +++ b/media/mojo/services/renderer_config_default.cc @@ -24,20 +24,6 @@ namespace media { namespace internal { -class DummyVideoRendererSink : public VideoRendererSink { - public: - DummyVideoRendererSink() {} - ~DummyVideoRendererSink() override {} - - void Start(RenderCallback* callback) override {} - void Stop() override {} - void PaintFrameUsingOldRenderingPath( - const scoped_refptr<VideoFrame>& frame) override {} - - private: - DISALLOW_COPY_AND_ASSIGN(DummyVideoRendererSink); -}; - class DefaultRendererConfig : public PlatformRendererConfig { public: DefaultRendererConfig() { @@ -101,10 +87,6 @@ class DefaultRendererConfig : public PlatformRendererConfig { return new AudioOutputStreamSink(); } - scoped_ptr<VideoRendererSink> GetVideoRendererSink() override { - return make_scoped_ptr(new DummyVideoRendererSink()); - } - const AudioHardwareConfig& GetAudioHardwareConfig() override { return *audio_hardware_config_; } diff --git a/media/renderers/default_renderer_factory.cc b/media/renderers/default_renderer_factory.cc index 0ec129a..81162d3 100644 --- a/media/renderers/default_renderer_factory.cc +++ b/media/renderers/default_renderer_factory.cc @@ -38,8 +38,7 @@ DefaultRendererFactory::~DefaultRendererFactory() { // TODO(xhwang): Use RendererConfig to customize what decoders we use. scoped_ptr<Renderer> DefaultRendererFactory::CreateRenderer( const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner, - AudioRendererSink* audio_renderer_sink, - VideoRendererSink* video_renderer_sink) { + AudioRendererSink* audio_renderer_sink) { DCHECK(audio_renderer_sink); // Create our audio decoders and renderer. @@ -76,9 +75,8 @@ scoped_ptr<Renderer> DefaultRendererFactory::CreateRenderer( video_decoders.push_back(new FFmpegVideoDecoder(media_task_runner)); #endif - scoped_ptr<VideoRenderer> video_renderer( - new VideoRendererImpl(media_task_runner, video_renderer_sink, - video_decoders.Pass(), true, media_log_)); + scoped_ptr<VideoRenderer> video_renderer(new VideoRendererImpl( + media_task_runner, video_decoders.Pass(), true, media_log_)); // Create renderer. return scoped_ptr<Renderer>(new RendererImpl( diff --git a/media/renderers/default_renderer_factory.h b/media/renderers/default_renderer_factory.h index 05cf2b1..0827992 100644 --- a/media/renderers/default_renderer_factory.h +++ b/media/renderers/default_renderer_factory.h @@ -15,7 +15,6 @@ class AudioHardwareConfig; class AudioRendererSink; class GpuVideoAcceleratorFactories; class MediaLog; -class VideoRendererSink; // The default factory class for creating RendererImpl. class MEDIA_EXPORT DefaultRendererFactory : public RendererFactory { @@ -28,8 +27,7 @@ class MEDIA_EXPORT DefaultRendererFactory : public RendererFactory { scoped_ptr<Renderer> CreateRenderer( const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner, - AudioRendererSink* audio_renderer_sink, - VideoRendererSink* video_renderer_sink) final; + AudioRendererSink* audio_renderer_sink) final; private: scoped_refptr<MediaLog> media_log_; diff --git a/media/renderers/renderer_impl.cc b/media/renderers/renderer_impl.cc index 62caf9d..ac80216 100644 --- a/media/renderers/renderer_impl.cc +++ b/media/renderers/renderer_impl.cc @@ -81,6 +81,7 @@ void RendererImpl::Initialize( const PipelineStatusCB& init_cb, const StatisticsCB& statistics_cb, const BufferingStateCB& buffering_state_cb, + const PaintCB& paint_cb, const base::Closure& ended_cb, const PipelineStatusCB& error_cb, const base::Closure& waiting_for_decryption_key_cb) { @@ -90,6 +91,7 @@ void RendererImpl::Initialize( DCHECK(!init_cb.is_null()); DCHECK(!statistics_cb.is_null()); DCHECK(!buffering_state_cb.is_null()); + DCHECK(!paint_cb.is_null()); DCHECK(!ended_cb.is_null()); DCHECK(!error_cb.is_null()); DCHECK(demuxer_stream_provider->GetStream(DemuxerStream::AUDIO) || @@ -98,6 +100,7 @@ void RendererImpl::Initialize( demuxer_stream_provider_ = demuxer_stream_provider; statistics_cb_ = statistics_cb; buffering_state_cb_ = buffering_state_cb; + paint_cb_ = paint_cb; ended_cb_ = ended_cb; error_cb_ = error_cb; init_cb_ = init_cb; @@ -334,6 +337,7 @@ void RendererImpl::InitializeVideoRenderer() { base::Bind(&RendererImpl::OnUpdateStatistics, weak_this_), base::Bind(&RendererImpl::OnBufferingStateChanged, weak_this_, &video_buffering_state_), + base::ResetAndReturn(&paint_cb_), base::Bind(&RendererImpl::OnVideoRendererEnded, weak_this_), base::Bind(&RendererImpl::OnError, weak_this_), base::Bind(&RendererImpl::GetWallClockTime, base::Unretained(this)), diff --git a/media/renderers/renderer_impl.h b/media/renderers/renderer_impl.h index 3f90271..feb6920 100644 --- a/media/renderers/renderer_impl.h +++ b/media/renderers/renderer_impl.h @@ -48,6 +48,7 @@ class MEDIA_EXPORT RendererImpl : public Renderer { const PipelineStatusCB& init_cb, const StatisticsCB& statistics_cb, const BufferingStateCB& buffering_state_cb, + const PaintCB& paint_cb, const base::Closure& ended_cb, const PipelineStatusCB& error_cb, const base::Closure& waiting_for_decryption_key_cb) final; @@ -139,6 +140,7 @@ class MEDIA_EXPORT RendererImpl : public Renderer { base::Closure ended_cb_; PipelineStatusCB error_cb_; BufferingStateCB buffering_state_cb_; + PaintCB paint_cb_; base::Closure waiting_for_decryption_key_cb_; // Temporary callback used for Initialize() and Flush(). diff --git a/media/renderers/renderer_impl_unittest.cc b/media/renderers/renderer_impl_unittest.cc index 9fcefe4..75fc050 100644 --- a/media/renderers/renderer_impl_unittest.cc +++ b/media/renderers/renderer_impl_unittest.cc @@ -49,6 +49,7 @@ class RendererImplTest : public ::testing::Test { MOCK_METHOD1(OnError, void(PipelineStatus)); MOCK_METHOD1(OnUpdateStatistics, void(const PipelineStatistics&)); MOCK_METHOD1(OnBufferingStateChange, void(BufferingState)); + MOCK_METHOD1(OnVideoFramePaint, void(const scoped_refptr<VideoFrame>&)); MOCK_METHOD0(OnWaitingForDecryptionKey, void()); private: @@ -97,9 +98,9 @@ class RendererImplTest : public ::testing::Test { // Sets up expectations to allow the video renderer to initialize. void SetVideoRendererInitializeExpectations(PipelineStatus status) { EXPECT_CALL(*video_renderer_, - Initialize(video_stream_.get(), _, _, _, _, _, _, _, _)) + Initialize(video_stream_.get(), _, _, _, _, _, _, _, _, _)) .WillOnce(DoAll(SaveArg<4>(&video_buffering_state_cb_), - SaveArg<5>(&video_ended_cb_), RunCallback<1>(status))); + SaveArg<6>(&video_ended_cb_), RunCallback<1>(status))); } void InitializeAndExpect(PipelineStatus start_status) { @@ -121,6 +122,8 @@ class RendererImplTest : public ::testing::Test { base::Unretained(&callbacks_)), base::Bind(&CallbackHelper::OnBufferingStateChange, base::Unretained(&callbacks_)), + base::Bind(&CallbackHelper::OnVideoFramePaint, + base::Unretained(&callbacks_)), base::Bind(&CallbackHelper::OnEnded, base::Unretained(&callbacks_)), base::Bind(&CallbackHelper::OnError, base::Unretained(&callbacks_)), base::Bind(&CallbackHelper::OnWaitingForDecryptionKey, @@ -472,10 +475,10 @@ TEST_F(RendererImplTest, ErrorDuringInitialize) { // Force an audio error to occur during video renderer initialization. EXPECT_CALL(*video_renderer_, - Initialize(video_stream_.get(), _, _, _, _, _, _, _, _)) + Initialize(video_stream_.get(), _, _, _, _, _, _, _, _, _)) .WillOnce(DoAll(AudioError(&audio_error_cb_, PIPELINE_ERROR_DECODE), SaveArg<4>(&video_buffering_state_cb_), - SaveArg<5>(&video_ended_cb_), + SaveArg<6>(&video_ended_cb_), RunCallback<1>(PIPELINE_OK))); InitializeAndExpect(PIPELINE_ERROR_DECODE); diff --git a/media/renderers/video_renderer_impl.cc b/media/renderers/video_renderer_impl.cc index 3e0698f..627ada6 100644 --- a/media/renderers/video_renderer_impl.cc +++ b/media/renderers/video_renderer_impl.cc @@ -22,12 +22,10 @@ namespace media { VideoRendererImpl::VideoRendererImpl( const scoped_refptr<base::SingleThreadTaskRunner>& task_runner, - VideoRendererSink* sink, ScopedVector<VideoDecoder> decoders, bool drop_frames, const scoped_refptr<MediaLog>& media_log) : task_runner_(task_runner), - sink_(sink), video_frame_stream_( new VideoFrameStream(task_runner, decoders.Pass(), media_log)), low_delay_(false), @@ -108,6 +106,7 @@ void VideoRendererImpl::Initialize( const SetDecryptorReadyCB& set_decryptor_ready_cb, const StatisticsCB& statistics_cb, const BufferingStateCB& buffering_state_cb, + const PaintCB& paint_cb, const base::Closure& ended_cb, const PipelineStatusCB& error_cb, const WallClockTimeCB& wall_clock_time_cb, @@ -119,6 +118,7 @@ void VideoRendererImpl::Initialize( DCHECK(!init_cb.is_null()); DCHECK(!statistics_cb.is_null()); DCHECK(!buffering_state_cb.is_null()); + DCHECK(!paint_cb.is_null()); DCHECK(!ended_cb.is_null()); DCHECK(!wall_clock_time_cb.is_null()); DCHECK_EQ(kUninitialized, state_); @@ -131,8 +131,7 @@ void VideoRendererImpl::Initialize( statistics_cb_ = statistics_cb; buffering_state_cb_ = buffering_state_cb; - paint_cb_ = base::Bind(&VideoRendererSink::PaintFrameUsingOldRenderingPath, - base::Unretained(sink_)); + paint_cb_ = paint_cb, ended_cb_ = ended_cb; error_cb_ = error_cb; wall_clock_time_cb_ = wall_clock_time_cb; @@ -144,19 +143,6 @@ void VideoRendererImpl::Initialize( set_decryptor_ready_cb, statistics_cb, waiting_for_decryption_key_cb); } -scoped_refptr<VideoFrame> VideoRendererImpl::Render( - base::TimeTicks deadline_min, - base::TimeTicks deadline_max) { - // TODO(dalecurtis): Hook this up to the new VideoRendererAlgorithm. - NOTIMPLEMENTED(); - return nullptr; -} - -void VideoRendererImpl::OnFrameDropped() { - // TODO(dalecurtis): Hook this up to the new VideoRendererAlgorithm. - NOTIMPLEMENTED(); -} - void VideoRendererImpl::CreateVideoThread() { // This may fail and cause a crash if there are too many threads created in // the current process. See http://crbug.com/443291 diff --git a/media/renderers/video_renderer_impl.h b/media/renderers/video_renderer_impl.h index 7eb9814..33938dc 100644 --- a/media/renderers/video_renderer_impl.h +++ b/media/renderers/video_renderer_impl.h @@ -21,7 +21,6 @@ #include "media/base/video_decoder.h" #include "media/base/video_frame.h" #include "media/base/video_renderer.h" -#include "media/base/video_renderer_sink.h" #include "media/filters/decoder_stream.h" namespace base { @@ -37,7 +36,6 @@ namespace media { // ready for rendering. class MEDIA_EXPORT VideoRendererImpl : public VideoRenderer, - public NON_EXPORTED_BASE(VideoRendererSink::RenderCallback), public base::PlatformThread::Delegate { public: // |decoders| contains the VideoDecoders to use when initializing. @@ -49,7 +47,6 @@ class MEDIA_EXPORT VideoRendererImpl // Setting |drop_frames_| to true causes the renderer to drop expired frames. VideoRendererImpl( const scoped_refptr<base::SingleThreadTaskRunner>& task_runner, - VideoRendererSink* sink, ScopedVector<VideoDecoder> decoders, bool drop_frames, const scoped_refptr<MediaLog>& media_log); @@ -61,6 +58,7 @@ class MEDIA_EXPORT VideoRendererImpl const SetDecryptorReadyCB& set_decryptor_ready_cb, const StatisticsCB& statistics_cb, const BufferingStateCB& buffering_state_cb, + const PaintCB& paint_cb, const base::Closure& ended_cb, const PipelineStatusCB& error_cb, const WallClockTimeCB& wall_clock_time_cb, @@ -74,11 +72,6 @@ class MEDIA_EXPORT VideoRendererImpl void SetTickClockForTesting(scoped_ptr<base::TickClock> tick_clock); - // VideoRendererSink::RenderCallback implementation. - scoped_refptr<VideoFrame> Render(base::TimeTicks deadline_min, - base::TimeTicks deadline_max) override; - void OnFrameDropped() override; - private: // Creates a dedicated |thread_| for video rendering. void CreateVideoThread(); @@ -125,8 +118,6 @@ class MEDIA_EXPORT VideoRendererImpl scoped_refptr<base::SingleThreadTaskRunner> task_runner_; - VideoRendererSink* const sink_; - // Used for accessing data members. base::Lock lock_; diff --git a/media/renderers/video_renderer_impl_unittest.cc b/media/renderers/video_renderer_impl_unittest.cc index 86f767f..df5c9d3 100644 --- a/media/renderers/video_renderer_impl_unittest.cc +++ b/media/renderers/video_renderer_impl_unittest.cc @@ -54,7 +54,6 @@ class VideoRendererImplTest : public ::testing::Test { decoders.push_back(decoder_); renderer_.reset(new VideoRendererImpl(message_loop_.message_loop_proxy(), - &mock_cb_, decoders.Pass(), true, new MediaLog())); renderer_->SetTickClockForTesting(scoped_ptr<base::TickClock>(tick_clock_)); @@ -109,6 +108,7 @@ class VideoRendererImplTest : public ::testing::Test { base::Unretained(this)), base::Bind(&StrictMock<MockCB>::BufferingStateChange, base::Unretained(&mock_cb_)), + base::Bind(&StrictMock<MockCB>::Display, base::Unretained(&mock_cb_)), ended_event_.GetClosure(), error_event_.GetPipelineStatusCB(), base::Bind(&VideoRendererImplTest::GetWallClockTime, base::Unretained(this)), @@ -265,15 +265,9 @@ class VideoRendererImplTest : public ::testing::Test { NiceMock<MockDemuxerStream> demuxer_stream_; // Use StrictMock<T> to catch missing/extra callbacks. - // TODO(dalecurtis): Mocks won't be useful for the new rendering path, we'll - // need fake callback generators like we have for the audio path. - // http://crbug.com/473424 - class MockCB : public VideoRendererSink { + class MockCB { public: - MOCK_METHOD1(Start, void(VideoRendererSink::RenderCallback*)); - MOCK_METHOD0(Stop, void()); - MOCK_METHOD1(PaintFrameUsingOldRenderingPath, - void(const scoped_refptr<VideoFrame>&)); + MOCK_METHOD1(Display, void(const scoped_refptr<VideoFrame>&)); MOCK_METHOD1(BufferingStateChange, void(BufferingState)); }; StrictMock<MockCB> mock_cb_; @@ -355,7 +349,7 @@ TEST_F(VideoRendererImplTest, Initialize) { TEST_F(VideoRendererImplTest, InitializeAndStartPlayingFrom) { Initialize(); QueueFrames("0 10 20 30"); - EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(0))); + EXPECT_CALL(mock_cb_, Display(HasTimestamp(0))); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH)); StartPlayingFrom(0); Destroy(); @@ -369,7 +363,7 @@ TEST_F(VideoRendererImplTest, DestroyWhileInitializing) { TEST_F(VideoRendererImplTest, DestroyWhileFlushing) { Initialize(); QueueFrames("0 10 20 30"); - EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(0))); + EXPECT_CALL(mock_cb_, Display(HasTimestamp(0))); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH)); StartPlayingFrom(0); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_NOTHING)); @@ -380,7 +374,7 @@ TEST_F(VideoRendererImplTest, DestroyWhileFlushing) { TEST_F(VideoRendererImplTest, Play) { Initialize(); QueueFrames("0 10 20 30"); - EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(0))); + EXPECT_CALL(mock_cb_, Display(HasTimestamp(0))); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH)); StartPlayingFrom(0); Destroy(); @@ -399,7 +393,7 @@ TEST_F(VideoRendererImplTest, FlushWithNothingBuffered) { TEST_F(VideoRendererImplTest, DecodeError_Playing) { Initialize(); QueueFrames("0 10 20 30"); - EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(0))); + EXPECT_CALL(mock_cb_, Display(HasTimestamp(0))); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH)); StartPlayingFrom(0); @@ -420,7 +414,7 @@ TEST_F(VideoRendererImplTest, StartPlayingFrom_Exact) { Initialize(); QueueFrames("50 60 70 80 90"); - EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(60))); + EXPECT_CALL(mock_cb_, Display(HasTimestamp(60))); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH)); StartPlayingFrom(60); Destroy(); @@ -430,7 +424,7 @@ TEST_F(VideoRendererImplTest, StartPlayingFrom_RightBefore) { Initialize(); QueueFrames("50 60 70 80 90"); - EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(50))); + EXPECT_CALL(mock_cb_, Display(HasTimestamp(50))); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH)); StartPlayingFrom(59); Destroy(); @@ -440,7 +434,7 @@ TEST_F(VideoRendererImplTest, StartPlayingFrom_RightAfter) { Initialize(); QueueFrames("50 60 70 80 90"); - EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(60))); + EXPECT_CALL(mock_cb_, Display(HasTimestamp(60))); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH)); StartPlayingFrom(61); Destroy(); @@ -452,7 +446,7 @@ TEST_F(VideoRendererImplTest, StartPlayingFrom_LowDelay) { QueueFrames("0"); // Expect some amount of have enough/nothing due to only requiring one frame. - EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(0))); + EXPECT_CALL(mock_cb_, Display(HasTimestamp(0))); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH)) .Times(AnyNumber()); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_NOTHING)) @@ -463,7 +457,7 @@ TEST_F(VideoRendererImplTest, StartPlayingFrom_LowDelay) { SatisfyPendingRead(); WaitableMessageLoopEvent event; - EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(10))) + EXPECT_CALL(mock_cb_, Display(HasTimestamp(10))) .WillOnce(RunClosure(event.GetClosure())); AdvanceTimeInMs(10); event.RunAndWait(); @@ -475,7 +469,7 @@ TEST_F(VideoRendererImplTest, StartPlayingFrom_LowDelay) { TEST_F(VideoRendererImplTest, DestroyDuringOutstandingRead) { Initialize(); QueueFrames("0 10 20 30"); - EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(0))); + EXPECT_CALL(mock_cb_, Display(HasTimestamp(0))); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH)); StartPlayingFrom(0); @@ -496,7 +490,7 @@ TEST_F(VideoRendererImplTest, Underflow) { { WaitableMessageLoopEvent event; - EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(0))); + EXPECT_CALL(mock_cb_, Display(HasTimestamp(0))); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH)) .WillOnce(RunClosure(event.GetClosure())); StartPlayingFrom(0); @@ -509,19 +503,16 @@ TEST_F(VideoRendererImplTest, Underflow) { { SCOPED_TRACE("Waiting for frame drops"); WaitableMessageLoopEvent event; - EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(10))) - .Times(0); - EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(20))) - .Times(0); - EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(30))) + EXPECT_CALL(mock_cb_, Display(HasTimestamp(10))).Times(0); + EXPECT_CALL(mock_cb_, Display(HasTimestamp(20))).Times(0); + EXPECT_CALL(mock_cb_, Display(HasTimestamp(30))) .WillOnce(RunClosure(event.GetClosure())); AdvanceTimeInMs(31); event.RunAndWait(); Mock::VerifyAndClearExpectations(&mock_cb_); } - // Advance time more. Now we should signal having nothing. And put - // the last frame up for display. + // Advance time more, such that a new frame should have been displayed by now. { SCOPED_TRACE("Waiting for BUFFERING_HAVE_NOTHING"); WaitableMessageLoopEvent event; diff --git a/media/test/pipeline_integration_test.cc b/media/test/pipeline_integration_test.cc index 6ce35f0..ead238c 100644 --- a/media/test/pipeline_integration_test.cc +++ b/media/test/pipeline_integration_test.cc @@ -631,8 +631,12 @@ class PipelineIntegrationTestHost : public mojo::test::ApplicationTestBase, void SetUp() override { ApplicationTestBase::SetUp(); - if (!IsMediaLibraryInitialized()) + + // TODO(dalecurtis): For some reason this isn't done... + if (!base::CommandLine::InitializedForCurrentProcess()) { + base::CommandLine::Init(0, NULL); InitializeMediaLibraryForTesting(); + } } protected: @@ -677,6 +681,8 @@ class PipelineIntegrationTest : public PipelineIntegrationTestHost { base::Unretained(this)), base::Bind(&PipelineIntegrationTest::OnBufferingStateChanged, base::Unretained(this)), + base::Bind(&PipelineIntegrationTest::OnVideoFramePaint, + base::Unretained(this)), base::Closure(), base::Bind(&PipelineIntegrationTest::OnAddTextTrack, base::Unretained(this)), base::Bind(&PipelineIntegrationTest::OnWaitingForDecryptionKey, @@ -721,6 +727,8 @@ class PipelineIntegrationTest : public PipelineIntegrationTestHost { base::Unretained(this)), base::Bind(&PipelineIntegrationTest::OnBufferingStateChanged, base::Unretained(this)), + base::Bind(&PipelineIntegrationTest::OnVideoFramePaint, + base::Unretained(this)), base::Closure(), base::Bind(&PipelineIntegrationTest::OnAddTextTrack, base::Unretained(this)), base::Bind(&PipelineIntegrationTest::OnWaitingForDecryptionKey, diff --git a/media/test/pipeline_integration_test_base.cc b/media/test/pipeline_integration_test_base.cc index c17431a..da3a9f8 100644 --- a/media/test/pipeline_integration_test_base.cc +++ b/media/test/pipeline_integration_test_base.cc @@ -26,7 +26,6 @@ using ::testing::_; using ::testing::AnyNumber; using ::testing::AtMost; -using ::testing::Invoke; using ::testing::InvokeWithoutArgs; using ::testing::SaveArg; @@ -35,9 +34,6 @@ namespace media { const char kNullVideoHash[] = "d41d8cd98f00b204e9800998ecf8427e"; const char kNullAudioHash[] = "0.00,0.00,0.00,0.00,0.00,0.00,"; -MockVideoRendererSink::MockVideoRendererSink() {} -MockVideoRendererSink::~MockVideoRendererSink() {} - PipelineIntegrationTestBase::PipelineIntegrationTestBase() : hashing_enabled_(false), clockless_playback_(false), @@ -139,6 +135,8 @@ PipelineStatus PipelineIntegrationTestBase::Start(const std::string& filename, base::Unretained(this)), base::Bind(&PipelineIntegrationTestBase::OnBufferingStateChanged, base::Unretained(this)), + base::Bind(&PipelineIntegrationTestBase::OnVideoFramePaint, + base::Unretained(this)), base::Closure(), base::Bind(&PipelineIntegrationTestBase::OnAddTextTrack, base::Unretained(this)), base::Bind(&PipelineIntegrationTestBase::OnWaitingForDecryptionKey, @@ -240,13 +238,9 @@ scoped_ptr<Renderer> PipelineIntegrationTestBase::CreateRenderer() { new FFmpegVideoDecoder(message_loop_.message_loop_proxy())); #endif - EXPECT_CALL(video_sink_, PaintFrameUsingOldRenderingPath(_)) - .WillRepeatedly( - Invoke(this, &PipelineIntegrationTestBase::OnVideoFramePaint)); - // Disable frame dropping if hashing is enabled. scoped_ptr<VideoRenderer> video_renderer( - new VideoRendererImpl(message_loop_.message_loop_proxy(), &video_sink_, + new VideoRendererImpl(message_loop_.message_loop_proxy(), video_decoders.Pass(), false, new MediaLog())); if (!clockless_playback_) { diff --git a/media/test/pipeline_integration_test_base.h b/media/test/pipeline_integration_test_base.h index aa41337..c8ca915 100644 --- a/media/test/pipeline_integration_test_base.h +++ b/media/test/pipeline_integration_test_base.h @@ -45,20 +45,6 @@ class DummyTickClock : public base::TickClock { base::TimeTicks now_; }; -// TODO(dalecurtis): Mocks won't be useful for the new rendering path, we'll -// need fake callback generators like we have for the audio path. -// http://crbug.com/473424 -class MockVideoRendererSink : public VideoRendererSink { - public: - MockVideoRendererSink(); - ~MockVideoRendererSink() override; - - MOCK_METHOD1(Start, void(VideoRendererSink::RenderCallback*)); - MOCK_METHOD0(Stop, void()); - MOCK_METHOD1(PaintFrameUsingOldRenderingPath, - void(const scoped_refptr<VideoFrame>&)); -}; - // Integration tests for Pipeline. Real demuxers, real decoders, and // base renderer implementations are used to verify pipeline functionality. The // renderers used in these tests rely heavily on the AudioRendererBase & @@ -119,7 +105,6 @@ class PipelineIntegrationTestBase { scoped_ptr<Pipeline> pipeline_; scoped_refptr<NullAudioSink> audio_sink_; scoped_refptr<ClocklessAudioSink> clockless_audio_sink_; - testing::NiceMock<MockVideoRendererSink> video_sink_; bool ended_; PipelineStatus pipeline_status_; Demuxer::EncryptedMediaInitDataCB encrypted_media_init_data_cb_; diff --git a/media/tools/player_x11/data_source_logger.cc b/media/tools/player_x11/data_source_logger.cc new file mode 100644 index 0000000..d09b6bf --- /dev/null +++ b/media/tools/player_x11/data_source_logger.cc @@ -0,0 +1,59 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "base/bind.h" +#include "base/logging.h" +#include "media/tools/player_x11/data_source_logger.h" + +static void LogAndRunReadCB( + int64 position, int size, + const media::DataSource::ReadCB& read_cb, int result) { + VLOG(1) << "Read(" << position << ", " << size << ") -> " << result; + read_cb.Run(result); +} + +DataSourceLogger::DataSourceLogger( + scoped_ptr<media::DataSource> data_source, + bool streaming) + : data_source_(data_source.Pass()), + streaming_(streaming) { +} + +void DataSourceLogger::Stop() { + VLOG(1) << "Stop()"; + data_source_->Stop(); +} + +void DataSourceLogger::Read( + int64 position, int size, uint8* data, + const media::DataSource::ReadCB& read_cb) { + VLOG(1) << "Read(" << position << ", " << size << ")"; + data_source_->Read(position, size, data, base::Bind( + &LogAndRunReadCB, position, size, read_cb)); +} + +bool DataSourceLogger::GetSize(int64* size_out) { + bool success = data_source_->GetSize(size_out); + VLOG(1) << "GetSize() -> " << (success ? "true" : "false") + << ", " << *size_out; + return success; +} + +bool DataSourceLogger::IsStreaming() { + if (streaming_) { + VLOG(1) << "IsStreaming() -> true (overridden)"; + return true; + } + + bool streaming = data_source_->IsStreaming(); + VLOG(1) << "IsStreaming() -> " << (streaming ? "true" : "false"); + return streaming; +} + +void DataSourceLogger::SetBitrate(int bitrate) { + VLOG(1) << "SetBitrate(" << bitrate << ")"; + data_source_->SetBitrate(bitrate); +} + +DataSourceLogger::~DataSourceLogger() {} diff --git a/media/tools/player_x11/data_source_logger.h b/media/tools/player_x11/data_source_logger.h new file mode 100644 index 0000000..13fdc60 --- /dev/null +++ b/media/tools/player_x11/data_source_logger.h @@ -0,0 +1,41 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef MEDIA_TOOLS_PLAYER_X11_DATA_SOURCE_LOGGER_H_ +#define MEDIA_TOOLS_PLAYER_X11_DATA_SOURCE_LOGGER_H_ + +#include "media/base/data_source.h" + +// Logs all DataSource operations to VLOG(1) for debugging purposes. +class DataSourceLogger : public media::DataSource { + public: + // Constructs a DataSourceLogger to log operations against another DataSource. + // + // |data_source| must be initialized in advance. + // + // |streaming| when set to true will override the implementation + // IsStreaming() to always return true, otherwise it will delegate to + // |data_source|. + DataSourceLogger(scoped_ptr<DataSource> data_source, + bool force_streaming); + ~DataSourceLogger() override; + + // media::DataSource implementation. + void Stop() override; + void Read(int64 position, + int size, + uint8* data, + const media::DataSource::ReadCB& read_cb) override; + bool GetSize(int64* size_out) override; + bool IsStreaming() override; + void SetBitrate(int bitrate) override; + + private: + scoped_ptr<media::DataSource> data_source_; + bool streaming_; + + DISALLOW_COPY_AND_ASSIGN(DataSourceLogger); +}; + +#endif // MEDIA_TOOLS_PLAYER_X11_DATA_SOURCE_LOGGER_H_ diff --git a/media/tools/player_x11/gl_video_renderer.cc b/media/tools/player_x11/gl_video_renderer.cc new file mode 100644 index 0000000..5f233c4 --- /dev/null +++ b/media/tools/player_x11/gl_video_renderer.cc @@ -0,0 +1,251 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "media/tools/player_x11/gl_video_renderer.h" + +#include <X11/Xutil.h> + +#include "base/bind.h" +#include "base/message_loop/message_loop.h" +#include "media/base/buffers.h" +#include "media/base/video_frame.h" +#include "media/base/yuv_convert.h" +#include "ui/gl/gl_surface.h" + +enum { kNumYUVPlanes = 3 }; + +static GLXContext InitGLContext(Display* display, Window window) { + // Some versions of NVIDIA's GL libGL.so include a broken version of + // dlopen/dlsym, and so linking it into chrome breaks it. So we dynamically + // load it, and use glew to dynamically resolve symbols. + // See http://code.google.com/p/chromium/issues/detail?id=16800 + if (!gfx::GLSurface::InitializeOneOff()) { + LOG(ERROR) << "GLSurface::InitializeOneOff failed"; + return NULL; + } + + XWindowAttributes attributes; + XGetWindowAttributes(display, window, &attributes); + XVisualInfo visual_info_template; + visual_info_template.visualid = XVisualIDFromVisual(attributes.visual); + int visual_info_count = 0; + XVisualInfo* visual_info_list = XGetVisualInfo(display, VisualIDMask, + &visual_info_template, + &visual_info_count); + GLXContext context = NULL; + for (int i = 0; i < visual_info_count && !context; ++i) { + context = glXCreateContext(display, visual_info_list + i, 0, + True /* Direct rendering */); + } + + XFree(visual_info_list); + if (!context) { + return NULL; + } + + if (!glXMakeCurrent(display, window, context)) { + glXDestroyContext(display, context); + return NULL; + } + + return context; +} + +// Matrix used for the YUV to RGB conversion. +static const float kYUV2RGB[9] = { + 1.f, 0.f, 1.403f, + 1.f, -.344f, -.714f, + 1.f, 1.772f, 0.f, +}; + +// Vertices for a full screen quad. +static const float kVertices[8] = { + -1.f, 1.f, + -1.f, -1.f, + 1.f, 1.f, + 1.f, -1.f, +}; + +// Pass-through vertex shader. +static const char kVertexShader[] = + "varying vec2 interp_tc;\n" + "\n" + "attribute vec4 in_pos;\n" + "attribute vec2 in_tc;\n" + "\n" + "void main() {\n" + " interp_tc = in_tc;\n" + " gl_Position = in_pos;\n" + "}\n"; + +// YUV to RGB pixel shader. Loads a pixel from each plane and pass through the +// matrix. +static const char kFragmentShader[] = + "varying vec2 interp_tc;\n" + "\n" + "uniform sampler2D y_tex;\n" + "uniform sampler2D u_tex;\n" + "uniform sampler2D v_tex;\n" + "uniform mat3 yuv2rgb;\n" + "\n" + "void main() {\n" + " float y = texture2D(y_tex, interp_tc).x;\n" + " float u = texture2D(u_tex, interp_tc).r - .5;\n" + " float v = texture2D(v_tex, interp_tc).r - .5;\n" + " vec3 rgb = yuv2rgb * vec3(y, u, v);\n" + " gl_FragColor = vec4(rgb, 1);\n" + "}\n"; + +// Buffer size for compile errors. +static const unsigned int kErrorSize = 4096; + +GlVideoRenderer::GlVideoRenderer(Display* display, Window window) + : display_(display), + window_(window), + gl_context_(NULL) { +} + +GlVideoRenderer::~GlVideoRenderer() { + glXMakeCurrent(display_, 0, NULL); + glXDestroyContext(display_, gl_context_); +} + +void GlVideoRenderer::Paint( + const scoped_refptr<media::VideoFrame>& video_frame) { + if (!gl_context_) + Initialize(video_frame->coded_size(), video_frame->visible_rect()); + + // Convert YUV frame to RGB. + DCHECK(video_frame->format() == media::VideoFrame::YV12 || + video_frame->format() == media::VideoFrame::I420 || + video_frame->format() == media::VideoFrame::YV16); + DCHECK(video_frame->stride(media::VideoFrame::kUPlane) == + video_frame->stride(media::VideoFrame::kVPlane)); + + if (glXGetCurrentContext() != gl_context_ || + glXGetCurrentDrawable() != window_) { + glXMakeCurrent(display_, window_, gl_context_); + } + for (unsigned int i = 0; i < kNumYUVPlanes; ++i) { + unsigned int width = video_frame->stride(i); + unsigned int height = video_frame->rows(i); + glActiveTexture(GL_TEXTURE0 + i); + glPixelStorei(GL_UNPACK_ROW_LENGTH, video_frame->stride(i)); + glTexImage2D(GL_TEXTURE_2D, 0, GL_LUMINANCE, width, height, 0, + GL_LUMINANCE, GL_UNSIGNED_BYTE, video_frame->data(i)); + } + + glDrawArrays(GL_TRIANGLE_STRIP, 0, 4); + glXSwapBuffers(display_, window_); +} + +void GlVideoRenderer::Initialize(gfx::Size coded_size, gfx::Rect visible_rect) { + CHECK(!gl_context_); + VLOG(0) << "Initializing GL Renderer..."; + + // Resize the window to fit that of the video. + XResizeWindow(display_, window_, visible_rect.width(), visible_rect.height()); + + gl_context_ = InitGLContext(display_, window_); + CHECK(gl_context_) << "Failed to initialize GL context"; + + // Create 3 textures, one for each plane, and bind them to different + // texture units. + glGenTextures(3, textures_); + glActiveTexture(GL_TEXTURE0); + glBindTexture(GL_TEXTURE_2D, textures_[0]); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST); + glEnable(GL_TEXTURE_2D); + + glActiveTexture(GL_TEXTURE1); + glBindTexture(GL_TEXTURE_2D, textures_[1]); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST); + glEnable(GL_TEXTURE_2D); + + glActiveTexture(GL_TEXTURE2); + glBindTexture(GL_TEXTURE_2D, textures_[2]); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST); + glEnable(GL_TEXTURE_2D); + + GLuint program = glCreateProgram(); + + // Create our YUV->RGB shader. + GLuint vertex_shader = glCreateShader(GL_VERTEX_SHADER); + const char* vs_source = kVertexShader; + int vs_size = sizeof(kVertexShader); + glShaderSource(vertex_shader, 1, &vs_source, &vs_size); + glCompileShader(vertex_shader); + int result = GL_FALSE; + glGetShaderiv(vertex_shader, GL_COMPILE_STATUS, &result); + if (!result) { + char log[kErrorSize]; + int len = 0; + glGetShaderInfoLog(vertex_shader, kErrorSize - 1, &len, log); + log[kErrorSize - 1] = 0; + LOG(FATAL) << log; + } + glAttachShader(program, vertex_shader); + glDeleteShader(vertex_shader); + + GLuint fragment_shader = glCreateShader(GL_FRAGMENT_SHADER); + const char* ps_source = kFragmentShader; + int ps_size = sizeof(kFragmentShader); + glShaderSource(fragment_shader, 1, &ps_source, &ps_size); + glCompileShader(fragment_shader); + result = GL_FALSE; + glGetShaderiv(fragment_shader, GL_COMPILE_STATUS, &result); + if (!result) { + char log[kErrorSize]; + int len = 0; + glGetShaderInfoLog(fragment_shader, kErrorSize - 1, &len, log); + log[kErrorSize - 1] = 0; + LOG(FATAL) << log; + } + glAttachShader(program, fragment_shader); + glDeleteShader(fragment_shader); + + glLinkProgram(program); + result = GL_FALSE; + glGetProgramiv(program, GL_LINK_STATUS, &result); + if (!result) { + char log[kErrorSize]; + int len = 0; + glGetProgramInfoLog(program, kErrorSize - 1, &len, log); + log[kErrorSize - 1] = 0; + LOG(FATAL) << log; + } + glUseProgram(program); + glDeleteProgram(program); + + // Bind parameters. + glUniform1i(glGetUniformLocation(program, "y_tex"), 0); + glUniform1i(glGetUniformLocation(program, "u_tex"), 1); + glUniform1i(glGetUniformLocation(program, "v_tex"), 2); + int yuv2rgb_location = glGetUniformLocation(program, "yuv2rgb"); + glUniformMatrix3fv(yuv2rgb_location, 1, GL_TRUE, kYUV2RGB); + + int pos_location = glGetAttribLocation(program, "in_pos"); + glEnableVertexAttribArray(pos_location); + glVertexAttribPointer(pos_location, 2, GL_FLOAT, GL_FALSE, 0, kVertices); + + int tc_location = glGetAttribLocation(program, "in_tc"); + glEnableVertexAttribArray(tc_location); + float verts[8]; + float x0 = static_cast<float>(visible_rect.x()) / coded_size.width(); + float y0 = static_cast<float>(visible_rect.y()) / coded_size.height(); + float x1 = static_cast<float>(visible_rect.right()) / coded_size.width(); + float y1 = static_cast<float>(visible_rect.bottom()) / coded_size.height(); + verts[0] = x0; verts[1] = y0; + verts[2] = x0; verts[3] = y1; + verts[4] = x1; verts[5] = y0; + verts[6] = x1; verts[7] = y1; + glVertexAttribPointer(tc_location, 2, GL_FLOAT, GL_FALSE, 0, verts); + + // We are getting called on a thread. Release the context so that it can be + // made current on the main thread. + glXMakeCurrent(display_, 0, NULL); +} diff --git a/media/tools/player_x11/gl_video_renderer.h b/media/tools/player_x11/gl_video_renderer.h new file mode 100644 index 0000000..a652eea --- /dev/null +++ b/media/tools/player_x11/gl_video_renderer.h @@ -0,0 +1,43 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef MEDIA_TOOLS_PLAYER_X11_GL_VIDEO_RENDERER_H_ +#define MEDIA_TOOLS_PLAYER_X11_GL_VIDEO_RENDERER_H_ + +#include "base/basictypes.h" +#include "base/memory/ref_counted.h" +#include "ui/gfx/geometry/rect.h" +#include "ui/gfx/geometry/size.h" +#include "ui/gl/gl_bindings.h" + +namespace media { +class VideoFrame; +} + +class GlVideoRenderer : public base::RefCountedThreadSafe<GlVideoRenderer> { + public: + GlVideoRenderer(Display* display, Window window); + + void Paint(const scoped_refptr<media::VideoFrame>& video_frame); + + private: + friend class base::RefCountedThreadSafe<GlVideoRenderer>; + ~GlVideoRenderer(); + + // Initializes GL rendering for the given dimensions. + void Initialize(gfx::Size coded_size, gfx::Rect visible_rect); + + Display* display_; + Window window_; + + // GL context. + GLXContext gl_context_; + + // 3 textures, one for each plane. + GLuint textures_[3]; + + DISALLOW_COPY_AND_ASSIGN(GlVideoRenderer); +}; + +#endif // MEDIA_TOOLS_PLAYER_X11_GL_VIDEO_RENDERER_H_ diff --git a/media/tools/player_x11/player_x11.cc b/media/tools/player_x11/player_x11.cc new file mode 100644 index 0000000..d149ca4 --- /dev/null +++ b/media/tools/player_x11/player_x11.cc @@ -0,0 +1,311 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include <signal.h> + +#include <iostream> // NOLINT + +#include "base/at_exit.h" +#include "base/bind.h" +#include "base/command_line.h" +#include "base/files/file_path.h" +#include "base/memory/scoped_ptr.h" +#include "base/memory/scoped_vector.h" +#include "base/threading/platform_thread.h" +#include "base/threading/thread.h" +#include "media/audio/audio_manager.h" +#include "media/audio/null_audio_sink.h" +#include "media/base/audio_hardware_config.h" +#include "media/base/bind_to_current_loop.h" +#include "media/base/decryptor.h" +#include "media/base/media.h" +#include "media/base/media_log.h" +#include "media/base/media_switches.h" +#include "media/base/pipeline.h" +#include "media/base/text_track.h" +#include "media/base/text_track_config.h" +#include "media/base/video_frame.h" +#include "media/filters/ffmpeg_audio_decoder.h" +#include "media/filters/ffmpeg_demuxer.h" +#include "media/filters/ffmpeg_video_decoder.h" +#include "media/filters/file_data_source.h" +#include "media/renderers/audio_renderer_impl.h" +#include "media/renderers/renderer_impl.h" +#include "media/renderers/video_renderer_impl.h" +#include "media/tools/player_x11/data_source_logger.h" + +// Include X11 headers here because X11/Xlib.h #define's Status +// which causes compiler errors with Status enum declarations +// in media::DemuxerStream & media::AudioDecoder. +#include <X11/XKBlib.h> +#include <X11/Xlib.h> + +#include "media/tools/player_x11/gl_video_renderer.h" +#include "media/tools/player_x11/x11_video_renderer.h" + +static Display* g_display = NULL; +static Window g_window = 0; +static bool g_running = false; + +media::AudioManager* g_audio_manager = NULL; + +scoped_ptr<media::DataSource> CreateDataSource(const std::string& file_path) { + media::FileDataSource* file_data_source = new media::FileDataSource(); + CHECK(file_data_source->Initialize(base::FilePath(file_path))); + + scoped_ptr<media::DataSource> data_source(file_data_source); + return data_source.Pass(); +} + +// Initialize X11. Returns true if successful. This method creates the X11 +// window. Further initialization is done in X11VideoRenderer. +bool InitX11() { + g_display = XOpenDisplay(NULL); + if (!g_display) { + std::cout << "Error - cannot open display" << std::endl; + return false; + } + + // Get properties of the screen. + int screen = DefaultScreen(g_display); + int root_window = RootWindow(g_display, screen); + + // Creates the window. + g_window = XCreateSimpleWindow(g_display, root_window, 1, 1, 100, 50, 0, + BlackPixel(g_display, screen), + BlackPixel(g_display, screen)); + XStoreName(g_display, g_window, "X11 Media Player"); + + XSelectInput(g_display, g_window, + ExposureMask | ButtonPressMask | KeyPressMask); + XMapWindow(g_display, g_window); + return true; +} + +static void DoNothing() {} + +static void OnStatus(media::PipelineStatus status) {} + +static void OnMetadata(media::PipelineMetadata metadata) {} + +static void OnBufferingStateChanged(media::BufferingState buffering_state) {} + +static void OnAddTextTrack(const media::TextTrackConfig& config, + const media::AddTextTrackDoneCB& done_cb) { +} + +static void OnEncryptedMediaInitData(media::EmeInitDataType init_data_type, + const std::vector<uint8>& init_data) { + std::cout << "File is encrypted." << std::endl; +} + +static void SaveStatusAndSignal(base::WaitableEvent* event, + media::PipelineStatus* status_out, + media::PipelineStatus status) { + *status_out = status; + event->Signal(); +} + +// TODO(vrk): Re-enabled audio. (crbug.com/112159) +void InitPipeline( + media::Pipeline* pipeline, + const scoped_refptr<base::SingleThreadTaskRunner>& task_runner, + media::Demuxer* demuxer, + const media::VideoRendererImpl::PaintCB& paint_cb, + bool /* enable_audio */) { + ScopedVector<media::VideoDecoder> video_decoders; + video_decoders.push_back(new media::FFmpegVideoDecoder(task_runner)); + scoped_ptr<media::VideoRenderer> video_renderer(new media::VideoRendererImpl( + task_runner, video_decoders.Pass(), true, new media::MediaLog())); + + ScopedVector<media::AudioDecoder> audio_decoders; + audio_decoders.push_back(new media::FFmpegAudioDecoder(task_runner, + media::LogCB())); + media::AudioParameters out_params( + media::AudioParameters::AUDIO_PCM_LOW_LATENCY, + media::CHANNEL_LAYOUT_STEREO, + 44100, + 16, + 512); + media::AudioHardwareConfig hardware_config(out_params, out_params); + + scoped_ptr<media::AudioRenderer> audio_renderer(new media::AudioRendererImpl( + task_runner, new media::NullAudioSink(task_runner), audio_decoders.Pass(), + hardware_config, new media::MediaLog())); + + scoped_ptr<media::Renderer> renderer(new media::RendererImpl( + task_runner, audio_renderer.Pass(), video_renderer.Pass())); + + base::WaitableEvent event(true, false); + media::PipelineStatus status; + + pipeline->Start(demuxer, + renderer.Pass(), + base::Bind(&DoNothing), + base::Bind(&OnStatus), + base::Bind(&SaveStatusAndSignal, &event, &status), + base::Bind(&OnMetadata), + base::Bind(&OnBufferingStateChanged), + paint_cb, + base::Bind(&DoNothing), + base::Bind(&OnAddTextTrack), + base::Bind(&DoNothing)); + + // Wait until the pipeline is fully initialized. + event.Wait(); + CHECK_EQ(status, media::PIPELINE_OK) << "Pipeline initialization failed"; + + // And start the playback. + pipeline->SetPlaybackRate(1.0f); +} + +void TerminateHandler(int signal) { + g_running = false; +} + +void PeriodicalUpdate( + media::Pipeline* pipeline, + base::MessageLoop* message_loop) { + if (!g_running) { + // interrupt signal was received during last time period. + // Quit message_loop only when pipeline is fully stopped. + pipeline->Stop(base::MessageLoop::QuitClosure()); + return; + } + + // Consume all the X events + while (XPending(g_display)) { + XEvent e; + XNextEvent(g_display, &e); + switch (e.type) { + case ButtonPress: + { + Window window; + int x, y; + unsigned int width, height, border_width, depth; + XGetGeometry(g_display, + g_window, + &window, + &x, + &y, + &width, + &height, + &border_width, + &depth); + base::TimeDelta time = pipeline->GetMediaDuration(); + pipeline->Seek(time*e.xbutton.x/width, base::Bind(&OnStatus)); + } + break; + case KeyPress: + { + KeySym key = XkbKeycodeToKeysym(g_display, e.xkey.keycode, 0, 0); + if (key == XK_Escape) { + g_running = false; + // Quit message_loop only when pipeline is fully stopped. + pipeline->Stop(base::MessageLoop::QuitClosure()); + return; + } else if (key == XK_space) { + if (pipeline->GetPlaybackRate() < 0.01f) // paused + pipeline->SetPlaybackRate(1.0f); + else + pipeline->SetPlaybackRate(0.0f); + } + } + break; + default: + break; + } + } + + message_loop->PostDelayedTask( + FROM_HERE, + base::Bind(&PeriodicalUpdate, + base::Unretained(pipeline), + message_loop), + base::TimeDelta::FromMilliseconds(10)); +} + +int main(int argc, char** argv) { + base::AtExitManager at_exit; + media::InitializeMediaLibraryForTesting(); + + base::CommandLine::Init(argc, argv); + base::CommandLine* command_line = base::CommandLine::ForCurrentProcess(); + std::string filename = command_line->GetSwitchValueASCII("file"); + + if (filename.empty()) { + std::cout << "Usage: " << argv[0] << " --file=FILE" << std::endl + << std::endl + << "Optional arguments:" << std::endl + << " [--audio]" + << " [--alsa-device=DEVICE]" + << " [--use-gl]" + << " [--streaming]" << std::endl + << " Press [ESC] to stop" << std::endl + << " Press [SPACE] to toggle pause/play" << std::endl + << " Press mouse left button to seek" << std::endl; + return 1; + } + + scoped_ptr<media::AudioManager> audio_manager( + media::AudioManager::CreateForTesting()); + g_audio_manager = audio_manager.get(); + + logging::LoggingSettings settings; + settings.logging_dest = logging::LOG_TO_SYSTEM_DEBUG_LOG; + logging::InitLogging(settings); + + // Install the signal handler. + signal(SIGTERM, &TerminateHandler); + signal(SIGINT, &TerminateHandler); + + // Initialize X11. + if (!InitX11()) + return 1; + + // Initialize the pipeline thread and the pipeline. + base::MessageLoop message_loop; + base::Thread media_thread("MediaThread"); + media_thread.Start(); + + media::VideoRendererImpl::PaintCB paint_cb; + if (command_line->HasSwitch("use-gl")) { + paint_cb = media::BindToCurrentLoop(base::Bind( + &GlVideoRenderer::Paint, new GlVideoRenderer(g_display, g_window))); + } else { + paint_cb = media::BindToCurrentLoop(base::Bind( + &X11VideoRenderer::Paint, new X11VideoRenderer(g_display, g_window))); + } + + scoped_ptr<media::DataSource> data_source(new DataSourceLogger( + CreateDataSource(filename), command_line->HasSwitch("streaming"))); + scoped_ptr<media::Demuxer> demuxer(new media::FFmpegDemuxer( + media_thread.message_loop_proxy(), data_source.get(), + base::Bind(&OnEncryptedMediaInitData), new media::MediaLog())); + + media::Pipeline pipeline(media_thread.message_loop_proxy(), + new media::MediaLog()); + InitPipeline(&pipeline, media_thread.message_loop_proxy(), demuxer.get(), + paint_cb, command_line->HasSwitch("audio")); + + // Main loop of the application. + g_running = true; + + message_loop.PostTask(FROM_HERE, base::Bind( + &PeriodicalUpdate, base::Unretained(&pipeline), &message_loop)); + message_loop.Run(); + + // Cleanup tasks. + media_thread.Stop(); + + // Release callback which releases video renderer. Do this before cleaning up + // X below since the video renderer has some X cleanup duties as well. + paint_cb.Reset(); + + XDestroyWindow(g_display, g_window); + XCloseDisplay(g_display); + g_audio_manager = NULL; + + return 0; +} diff --git a/media/tools/player_x11/x11_video_renderer.cc b/media/tools/player_x11/x11_video_renderer.cc new file mode 100644 index 0000000..2ae8e3b --- /dev/null +++ b/media/tools/player_x11/x11_video_renderer.cc @@ -0,0 +1,215 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "media/tools/player_x11/x11_video_renderer.h" + +#include <dlfcn.h> +#include <X11/Xutil.h> +#include <X11/extensions/Xrender.h> +#include <X11/extensions/Xcomposite.h> + +#include "base/bind.h" +#include "base/message_loop/message_loop.h" +#include "media/base/video_frame.h" +#include "media/base/yuv_convert.h" + +// Creates a 32-bit XImage. +static XImage* CreateImage(Display* display, int width, int height) { + VLOG(0) << "Allocating XImage " << width << "x" << height; + return XCreateImage(display, + DefaultVisual(display, DefaultScreen(display)), + DefaultDepth(display, DefaultScreen(display)), + ZPixmap, + 0, + static_cast<char*>(malloc(width * height * 4)), + width, + height, + 32, + width * 4); +} + +// Returns the picture format for ARGB. +// This method is originally from chrome/common/x11_util.cc. +static XRenderPictFormat* GetRenderARGB32Format(Display* dpy) { + static XRenderPictFormat* pictformat = NULL; + if (pictformat) + return pictformat; + + // First look for a 32-bit format which ignores the alpha value. + XRenderPictFormat templ; + templ.depth = 32; + templ.type = PictTypeDirect; + templ.direct.red = 16; + templ.direct.green = 8; + templ.direct.blue = 0; + templ.direct.redMask = 0xff; + templ.direct.greenMask = 0xff; + templ.direct.blueMask = 0xff; + templ.direct.alphaMask = 0; + + static const unsigned long kMask = + PictFormatType | PictFormatDepth | + PictFormatRed | PictFormatRedMask | + PictFormatGreen | PictFormatGreenMask | + PictFormatBlue | PictFormatBlueMask | + PictFormatAlphaMask; + + pictformat = XRenderFindFormat(dpy, kMask, &templ, 0 /* first result */); + + if (!pictformat) { + // Not all X servers support xRGB32 formats. However, the XRender spec + // says that they must support an ARGB32 format, so we can always return + // that. + pictformat = XRenderFindStandardFormat(dpy, PictStandardARGB32); + CHECK(pictformat) << "XRender ARGB32 not supported."; + } + + return pictformat; +} + +X11VideoRenderer::X11VideoRenderer(Display* display, Window window) + : display_(display), + window_(window), + image_(NULL), + picture_(0), + use_render_(false) { +} + +X11VideoRenderer::~X11VideoRenderer() { + if (image_) + XDestroyImage(image_); + if (use_render_) + XRenderFreePicture(display_, picture_); +} + +void X11VideoRenderer::Paint( + const scoped_refptr<media::VideoFrame>& video_frame) { + if (!image_) + Initialize(video_frame->coded_size(), video_frame->visible_rect()); + + const int coded_width = video_frame->coded_size().width(); + const int coded_height = video_frame->coded_size().height(); + const int visible_width = video_frame->visible_rect().width(); + const int visible_height = video_frame->visible_rect().height(); + + // Check if we need to reallocate our XImage. + if (image_->width != coded_width || image_->height != coded_height) { + XDestroyImage(image_); + image_ = CreateImage(display_, coded_width, coded_height); + } + + // Convert YUV frame to RGB. + DCHECK(video_frame->format() == media::VideoFrame::YV12 || + video_frame->format() == media::VideoFrame::I420 || + video_frame->format() == media::VideoFrame::YV16); + DCHECK(video_frame->stride(media::VideoFrame::kUPlane) == + video_frame->stride(media::VideoFrame::kVPlane)); + + DCHECK(image_->data); + media::YUVType yuv_type = (video_frame->format() == media::VideoFrame::YV12 || + video_frame->format() == media::VideoFrame::I420) + ? media::YV12 + : media::YV16; + media::ConvertYUVToRGB32(video_frame->data(media::VideoFrame::kYPlane), + video_frame->data(media::VideoFrame::kUPlane), + video_frame->data(media::VideoFrame::kVPlane), + (uint8*)image_->data, coded_width, coded_height, + video_frame->stride(media::VideoFrame::kYPlane), + video_frame->stride(media::VideoFrame::kUPlane), + image_->bytes_per_line, + yuv_type); + + if (use_render_) { + // If XRender is used, we'll upload the image to a pixmap. And then + // creats a picture from the pixmap and composite the picture over + // the picture represending the window. + + // Creates a XImage. + XImage image; + memset(&image, 0, sizeof(image)); + image.width = coded_width; + image.height = coded_height; + image.depth = 32; + image.bits_per_pixel = 32; + image.format = ZPixmap; + image.byte_order = LSBFirst; + image.bitmap_unit = 8; + image.bitmap_bit_order = LSBFirst; + image.bytes_per_line = image_->bytes_per_line; + image.red_mask = 0xff; + image.green_mask = 0xff00; + image.blue_mask = 0xff0000; + image.data = image_->data; + + // Creates a pixmap and uploads from the XImage. + unsigned long pixmap = XCreatePixmap(display_, window_, + visible_width, visible_height, + 32); + GC gc = XCreateGC(display_, pixmap, 0, NULL); + XPutImage(display_, pixmap, gc, &image, + video_frame->visible_rect().x(), + video_frame->visible_rect().y(), + 0, 0, + visible_width, visible_height); + XFreeGC(display_, gc); + + // Creates the picture representing the pixmap. + unsigned long picture = XRenderCreatePicture( + display_, pixmap, GetRenderARGB32Format(display_), 0, NULL); + + // Composite the picture over the picture representing the window. + XRenderComposite(display_, PictOpSrc, picture, 0, + picture_, 0, 0, 0, 0, 0, 0, + visible_width, visible_height); + + XRenderFreePicture(display_, picture); + XFreePixmap(display_, pixmap); + return; + } + + // If XRender is not used, simply put the image to the server. + // This will have a tearing effect but this is OK. + // TODO(hclam): Upload the image to a pixmap and do XCopyArea() + // to the window. + GC gc = XCreateGC(display_, window_, 0, NULL); + XPutImage(display_, window_, gc, image_, + video_frame->visible_rect().x(), + video_frame->visible_rect().y(), + 0, 0, visible_width, visible_height); + XFlush(display_); + XFreeGC(display_, gc); +} + +void X11VideoRenderer::Initialize(gfx::Size coded_size, + gfx::Rect visible_rect) { + CHECK(!image_); + VLOG(0) << "Initializing X11 Renderer..."; + + // Resize the window to fit that of the video. + XResizeWindow(display_, window_, visible_rect.width(), visible_rect.height()); + image_ = CreateImage(display_, coded_size.width(), coded_size.height()); + + // Testing XRender support. We'll use the very basic of XRender + // so if it presents it is already good enough. We don't need + // to check its version. + int dummy; + use_render_ = XRenderQueryExtension(display_, &dummy, &dummy); + + if (use_render_) { + VLOG(0) << "Using XRender extension."; + + // If we are using XRender, we'll create a picture representing the + // window. + XWindowAttributes attr; + XGetWindowAttributes(display_, window_, &attr); + + XRenderPictFormat* pictformat = XRenderFindVisualFormat( + display_, + attr.visual); + CHECK(pictformat) << "XRender does not support default visual"; + + picture_ = XRenderCreatePicture(display_, window_, pictformat, 0, NULL); + CHECK(picture_) << "Backing picture not created"; + } +} diff --git a/media/tools/player_x11/x11_video_renderer.h b/media/tools/player_x11/x11_video_renderer.h new file mode 100644 index 0000000..11213c3 --- /dev/null +++ b/media/tools/player_x11/x11_video_renderer.h @@ -0,0 +1,47 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef MEDIA_TOOLS_PLAYER_X11_X11_VIDEO_RENDERER_H_ +#define MEDIA_TOOLS_PLAYER_X11_X11_VIDEO_RENDERER_H_ + +#include <X11/Xlib.h> + +#include "base/basictypes.h" +#include "base/memory/ref_counted.h" +#include "ui/gfx/geometry/rect.h" +#include "ui/gfx/geometry/size.h" + +namespace media { +class VideoFrame; +} + +class X11VideoRenderer : public base::RefCountedThreadSafe<X11VideoRenderer> { + public: + X11VideoRenderer(Display* display, Window window); + + void Paint(const scoped_refptr<media::VideoFrame>& video_frame); + + private: + friend class base::RefCountedThreadSafe<X11VideoRenderer>; + ~X11VideoRenderer(); + + // Initializes X11 rendering for the given dimensions. + void Initialize(gfx::Size coded_size, gfx::Rect visible_rect); + + Display* display_; + Window window_; + + // Image in heap that contains the RGBA data of the video frame. + XImage* image_; + + // Picture represents the paint target. This is a picture located + // in the server. + unsigned long picture_; + + bool use_render_; + + DISALLOW_COPY_AND_ASSIGN(X11VideoRenderer); +}; + +#endif // MEDIA_TOOLS_PLAYER_X11_X11_VIDEO_RENDERER_H_ diff --git a/net/url_request/url_fetcher_impl_unittest.cc b/net/url_request/url_fetcher_impl_unittest.cc index 1212519..3bf1a36 100644 --- a/net/url_request/url_fetcher_impl_unittest.cc +++ b/net/url_request/url_fetcher_impl_unittest.cc @@ -200,58 +200,6 @@ class URLFetcherTest : public testing::Test, return num_upload_streams_created_; } - // Downloads |file_to_fetch| and checks the contents when done. If - // |save_to_temporary_file| is true, saves it to a temporary file, and - // |requested_out_path| is ignored. Otherwise, saves it to - // |requested_out_path|. Takes ownership of the file if |take_ownership| is - // true. Deletes file when done. - void SaveFileTest(const char* file_to_fetch, - bool save_to_temporary_file, - const base::FilePath& requested_out_path, - bool take_ownership) { - scoped_ptr<WaitingURLFetcherDelegate> delegate( - new WaitingURLFetcherDelegate()); - delegate->CreateFetcherWithContext( - test_server_->GetURL(std::string(kTestServerFilePrefix) + - file_to_fetch), - URLFetcher::GET, request_context()); - if (save_to_temporary_file) { - delegate->fetcher()->SaveResponseToTemporaryFile( - scoped_refptr<base::MessageLoopProxy>( - base::MessageLoopProxy::current())); - } else { - delegate->fetcher()->SaveResponseToFileAtPath( - requested_out_path, scoped_refptr<base::MessageLoopProxy>( - base::MessageLoopProxy::current())); - } - delegate->StartFetcherAndWait(); - - EXPECT_TRUE(delegate->fetcher()->GetStatus().is_success()); - EXPECT_EQ(200, delegate->fetcher()->GetResponseCode()); - - base::FilePath out_path; - EXPECT_TRUE( - delegate->fetcher()->GetResponseAsFilePath(take_ownership, &out_path)); - if (!save_to_temporary_file) { - EXPECT_EQ(requested_out_path, out_path); - } - - EXPECT_TRUE(base::ContentsEqual( - test_server_->GetDocumentRoot().AppendASCII(file_to_fetch), out_path)); - - // Delete the delegate and run the message loop to give the fetcher's - // destructor a chance to delete the file. - delegate.reset(); - base::RunLoop().RunUntilIdle(); - - // File should only exist if |take_ownership| was true. - EXPECT_EQ(take_ownership, base::PathExists(out_path)); - - // Cleanup. - if (base::PathExists(out_path)) - base::DeleteFile(out_path, false); - } - // Returns a URL that hangs on DNS resolution. Only hangs when using the // request context returned by request_context(). const GURL& hanging_url() const { return hanging_url_; } @@ -516,6 +464,30 @@ class URLFetcherMultipleAttemptTest : public URLFetcherTest { std::string data_; }; +class URLFetcherFileTest : public URLFetcherTest { + public: + URLFetcherFileTest() : take_ownership_of_file_(false), + expected_file_error_(OK) {} + + void CreateFetcherForFile(const GURL& url, const base::FilePath& file_path); + void CreateFetcherForTempFile(const GURL& url); + + // URLFetcherDelegate: + void OnURLFetchComplete(const URLFetcher* source) override; + + protected: + base::FilePath expected_file_; + base::FilePath file_path_; + + // Set by the test. Used in OnURLFetchComplete() to decide if + // the URLFetcher should own the temp file, so that we can test + // disowning prevents the file from being deleted. + bool take_ownership_of_file_; + + // Expected file error code for the test. OK when expecting success. + int expected_file_error_; +}; + void URLFetcherDownloadProgressTest::CreateFetcher(const GURL& url) { fetcher_ = new URLFetcherImpl(url, URLFetcher::GET, this); fetcher_->SetRequestContext(new ThrottlingTestURLRequestContextGetter( @@ -665,6 +637,44 @@ void URLFetcherMultipleAttemptTest::OnURLFetchComplete( } } +void URLFetcherFileTest::CreateFetcherForFile(const GURL& url, + const base::FilePath& file_path) { + fetcher_ = new URLFetcherImpl(url, URLFetcher::GET, this); + fetcher_->SetRequestContext(new ThrottlingTestURLRequestContextGetter( + io_message_loop_proxy().get(), request_context())); + + // Use the IO message loop to do the file operations in this test. + fetcher_->SaveResponseToFileAtPath(file_path, io_message_loop_proxy()); + fetcher_->Start(); +} + +void URLFetcherFileTest::CreateFetcherForTempFile(const GURL& url) { + fetcher_ = new URLFetcherImpl(url, URLFetcher::GET, this); + fetcher_->SetRequestContext(new ThrottlingTestURLRequestContextGetter( + io_message_loop_proxy().get(), request_context())); + + // Use the IO message loop to do the file operations in this test. + fetcher_->SaveResponseToTemporaryFile(io_message_loop_proxy()); + fetcher_->Start(); +} + +void URLFetcherFileTest::OnURLFetchComplete(const URLFetcher* source) { + if (expected_file_error_ == OK) { + EXPECT_TRUE(source->GetStatus().is_success()); + EXPECT_EQ(OK, source->GetStatus().error()); + EXPECT_EQ(200, source->GetResponseCode()); + + EXPECT_TRUE(source->GetResponseAsFilePath( + take_ownership_of_file_, &file_path_)); + + EXPECT_TRUE(base::ContentsEqual(expected_file_, file_path_)); + } else { + EXPECT_FALSE(source->GetStatus().is_success()); + EXPECT_EQ(expected_file_error_, source->GetStatus().error()); + } + CleanupAfterFetchComplete(); +} + // Create the fetcher on the main thread. Since network IO will happen on the // main thread, this will test URLFetcher's ability to do everything on one // thread. @@ -1192,93 +1202,149 @@ TEST_F(URLFetcherMultipleAttemptTest, SameData) { base::MessageLoop::current()->Run(); } -// Get a small file. -TEST_F(URLFetcherTest, FileTestSmallGet) { - const char kFileToFetch[] = "simple.html"; - +TEST_F(URLFetcherFileTest, SmallGet) { base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - base::FilePath out_path = temp_dir.path().AppendASCII(kFileToFetch); - SaveFileTest(kFileToFetch, false, out_path, false); -} -// Get a file large enough to require more than one read into URLFetcher::Core's -// IOBuffer. -TEST_F(URLFetcherTest, FileTestLargeGet) { - const char kFileToFetch[] = "animate1.gif"; + // Get a small file. + static const char kFileToFetch[] = "simple.html"; + expected_file_ = test_server_->GetDocumentRoot().AppendASCII(kFileToFetch); + CreateFetcherForFile( + test_server_->GetURL(std::string(kTestServerFilePrefix) + kFileToFetch), + temp_dir.path().AppendASCII(kFileToFetch)); - base::ScopedTempDir temp_dir; - ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - base::FilePath out_path = temp_dir.path().AppendASCII(kFileToFetch); - SaveFileTest(kFileToFetch, false, out_path, false); -} + base::MessageLoop::current()->Run(); // OnURLFetchComplete() will Quit(). -// If the caller takes the ownership of the output file, the file should persist -// even after URLFetcher is gone. -TEST_F(URLFetcherTest, FileTestTakeOwnership) { - const char kFileToFetch[] = "simple.html"; + ASSERT_FALSE(base::PathExists(file_path_)) + << file_path_.value() << " not removed."; +} +TEST_F(URLFetcherFileTest, LargeGet) { base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - base::FilePath out_path = temp_dir.path().AppendASCII(kFileToFetch); - SaveFileTest(kFileToFetch, false, out_path, true); + + // Get a file large enough to require more than one read into + // URLFetcher::Core's IOBuffer. + static const char kFileToFetch[] = "animate1.gif"; + expected_file_ = test_server_->GetDocumentRoot().AppendASCII(kFileToFetch); + CreateFetcherForFile( + test_server_->GetURL(std::string(kTestServerFilePrefix) + kFileToFetch), + temp_dir.path().AppendASCII(kFileToFetch)); + + base::MessageLoop::current()->Run(); // OnURLFetchComplete() will Quit(). } -// Test that an existing file can be overwritten be a fetcher. -TEST_F(URLFetcherTest, FileTestOverwriteExisting) { +TEST_F(URLFetcherFileTest, SavedOutputFileOwnerhisp) { + // If the caller takes the ownership of the output file, the file should + // persist even after URLFetcher is gone. If not, the file must be deleted. + const bool kTake[] = {false, true}; + for (size_t i = 0; i < arraysize(kTake); ++i) { + take_ownership_of_file_ = kTake[i]; + base::ScopedTempDir temp_dir; + ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); + + // Get a small file. + static const char kFileToFetch[] = "simple.html"; + expected_file_ = test_server_->GetDocumentRoot().AppendASCII(kFileToFetch); + CreateFetcherForFile( + test_server_->GetURL(std::string(kTestServerFilePrefix) + kFileToFetch), + temp_dir.path().AppendASCII(kFileToFetch)); + + base::MessageLoop::current()->Run(); // OnURLFetchComplete() will Quit(). + + base::MessageLoop::current()->RunUntilIdle(); + ASSERT_EQ(kTake[i], base::PathExists(file_path_)) << + "FilePath: " << file_path_.value(); + } +} + +TEST_F(URLFetcherFileTest, OverwriteExistingFile) { base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); // Create a file before trying to fetch. - const char kFileToFetch[] = "simple.html"; + static const char kFileToFetch[] = "simple.html"; std::string data(10000, '?'); // Meant to be larger than simple.html. - base::FilePath out_path = temp_dir.path().AppendASCII(kFileToFetch); + file_path_ = temp_dir.path().AppendASCII(kFileToFetch); ASSERT_EQ(static_cast<int>(data.size()), - base::WriteFile(out_path, data.data(), data.size())); - ASSERT_TRUE(base::PathExists(out_path)); + base::WriteFile(file_path_, data.data(), data.size())); + ASSERT_TRUE(base::PathExists(file_path_)); + expected_file_ = test_server_->GetDocumentRoot().AppendASCII(kFileToFetch); + ASSERT_FALSE(base::ContentsEqual(file_path_, expected_file_)); - SaveFileTest(kFileToFetch, false, out_path, true); + // Get a small file. + CreateFetcherForFile( + test_server_->GetURL(std::string(kTestServerFilePrefix) + kFileToFetch), + file_path_); + + base::MessageLoop::current()->Run(); // OnURLFetchComplete() will Quit(). } -// Test trying to overwrite a directory with a file when using a fetcher fails. -TEST_F(URLFetcherTest, FileTestTryToOverwriteDirectory) { +TEST_F(URLFetcherFileTest, TryToOverwriteDirectory) { base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); // Create a directory before trying to fetch. static const char kFileToFetch[] = "simple.html"; - base::FilePath out_path = temp_dir.path().AppendASCII(kFileToFetch); - ASSERT_TRUE(base::CreateDirectory(out_path)); - ASSERT_TRUE(base::PathExists(out_path)); - - WaitingURLFetcherDelegate delegate; - delegate.CreateFetcherWithContext( + file_path_ = temp_dir.path().AppendASCII(kFileToFetch); + ASSERT_TRUE(base::CreateDirectory(file_path_)); + ASSERT_TRUE(base::PathExists(file_path_)); + + // Get a small file. + expected_file_error_ = ERR_ACCESS_DENIED; + expected_file_ = test_server_->GetDocumentRoot().AppendASCII(kFileToFetch); + CreateFetcherForFile( test_server_->GetURL(std::string(kTestServerFilePrefix) + kFileToFetch), - URLFetcher::GET, request_context()); - delegate.fetcher()->SaveResponseToFileAtPath( - out_path, - scoped_refptr<base::MessageLoopProxy>(base::MessageLoopProxy::current())); - delegate.StartFetcherAndWait(); + file_path_); + + base::MessageLoop::current()->Run(); // OnURLFetchComplete() will Quit(). - EXPECT_FALSE(delegate.fetcher()->GetStatus().is_success()); - EXPECT_EQ(ERR_ACCESS_DENIED, delegate.fetcher()->GetStatus().error()); + base::MessageLoop::current()->RunUntilIdle(); } -// Get a small file and save it to a temp file. -TEST_F(URLFetcherTest, TempFileTestSmallGet) { - SaveFileTest("simple.html", true, base::FilePath(), false); +TEST_F(URLFetcherFileTest, SmallGetToTempFile) { + // Get a small file. + static const char kFileToFetch[] = "simple.html"; + expected_file_ = test_server_->GetDocumentRoot().AppendASCII(kFileToFetch); + CreateFetcherForTempFile( + test_server_->GetURL(std::string(kTestServerFilePrefix) + kFileToFetch)); + + base::MessageLoop::current()->Run(); // OnURLFetchComplete() will Quit(). + + ASSERT_FALSE(base::PathExists(file_path_)) + << file_path_.value() << " not removed."; } -// Get a file large enough to require more than one read into URLFetcher::Core's -// IOBuffer and save it to a temp file. -TEST_F(URLFetcherTest, TempFileTestLargeGet) { - SaveFileTest("animate1.gif", true, base::FilePath(), false); +TEST_F(URLFetcherFileTest, LargeGetToTempFile) { + // Get a file large enough to require more than one read into + // URLFetcher::Core's IOBuffer. + static const char kFileToFetch[] = "animate1.gif"; + expected_file_ = test_server_->GetDocumentRoot().AppendASCII(kFileToFetch); + CreateFetcherForTempFile( + test_server_->GetURL(std::string(kTestServerFilePrefix) + kFileToFetch)); + + base::MessageLoop::current()->Run(); // OnURLFetchComplete() will Quit(). } -// If the caller takes the ownership of the temp file, check that the file -// persists even after URLFetcher is gone. -TEST_F(URLFetcherTest, TempFileTestTakeOwnership) { - SaveFileTest("simple.html", true, base::FilePath(), true); +TEST_F(URLFetcherFileTest, SavedOutputTempFileOwnerhisp) { + // If the caller takes the ownership of the temp file, the file should persist + // even after URLFetcher is gone. If not, the file must be deleted. + const bool kTake[] = {false, true}; + for (size_t i = 0; i < arraysize(kTake); ++i) { + take_ownership_of_file_ = kTake[i]; + + // Get a small file. + static const char kFileToFetch[] = "simple.html"; + expected_file_ = test_server_->GetDocumentRoot().AppendASCII(kFileToFetch); + CreateFetcherForTempFile(test_server_->GetURL( + std::string(kTestServerFilePrefix) + kFileToFetch)); + + base::MessageLoop::current()->Run(); // OnURLFetchComplete() will Quit(). + + base::MessageLoop::current()->RunUntilIdle(); + ASSERT_EQ(kTake[i], base::PathExists(file_path_)) << + "FilePath: " << file_path_.value(); + } } } // namespace diff --git a/net/url_request/url_request_test_util.h b/net/url_request/url_request_test_util.h index c856c98..0208b43 100644 --- a/net/url_request/url_request_test_util.h +++ b/net/url_request/url_request_test_util.h @@ -70,8 +70,7 @@ class TestURLRequestContext : public URLRequestContext { } void set_http_network_session_params( - scoped_ptr<HttpNetworkSession::Params> params) { - http_network_session_params_ = params.Pass(); + const HttpNetworkSession::Params& params) { } void SetSdchManager(scoped_ptr<SdchManager> sdch_manager) { diff --git a/sync/BUILD.gn b/sync/BUILD.gn index 9a95bb1..cd6e0e4 100644 --- a/sync/BUILD.gn +++ b/sync/BUILD.gn @@ -819,7 +819,6 @@ if (is_android) { "//base", "//sync/protocol:protocol", "//testing/gtest", - "//url:url", ] } } diff --git a/sync/test/fake_server/android/fake_server_helper_android.cc b/sync/test/fake_server/android/fake_server_helper_android.cc index 79c0bab..cc8fe81 100644 --- a/sync/test/fake_server/android/fake_server_helper_android.cc +++ b/sync/test/fake_server/android/fake_server_helper_android.cc @@ -14,14 +14,11 @@ #include "sync/internal_api/public/base/model_type.h" #include "sync/internal_api/public/network_resources.h" #include "sync/protocol/sync.pb.h" -#include "sync/test/fake_server/bookmark_entity_builder.h" -#include "sync/test/fake_server/entity_builder_factory.h" #include "sync/test/fake_server/fake_server.h" #include "sync/test/fake_server/fake_server_network_resources.h" #include "sync/test/fake_server/fake_server_verifier.h" #include "sync/test/fake_server/unique_client_entity.h" #include "testing/gtest/include/gtest/gtest.h" -#include "url/gurl.h" FakeServerHelperAndroid::FakeServerHelperAndroid(JNIEnv* env, jobject obj) { } @@ -108,44 +105,6 @@ void FakeServerHelperAndroid::InjectUniqueClientEntity( entity_specifics)); } -void FakeServerHelperAndroid::InjectBookmarkEntity( - JNIEnv* env, - jobject obj, - jlong fake_server, - jstring title, - jstring url, - jstring parent_id) { - fake_server::FakeServer* fake_server_ptr = - reinterpret_cast<fake_server::FakeServer*>(fake_server); - - std::string url_as_string = base::android::ConvertJavaStringToUTF8(env, url); - GURL gurl = GURL(url_as_string); - if (!gurl.is_valid()) { - NOTREACHED() << "The given string (" << url_as_string - << ") is not a valid URL."; - } - - fake_server::EntityBuilderFactory entity_builder_factory; - fake_server::BookmarkEntityBuilder bookmark_builder = - entity_builder_factory.NewBookmarkEntityBuilder( - base::android::ConvertJavaStringToUTF8(env, title), gurl); - bookmark_builder.SetParentId( - base::android::ConvertJavaStringToUTF8(env, parent_id)); - scoped_ptr<fake_server::FakeServerEntity> bookmark = bookmark_builder.Build(); - fake_server_ptr->InjectEntity(bookmark.Pass()); -} - -base::android::ScopedJavaLocalRef<jstring> -FakeServerHelperAndroid::GetBookmarkBarFolderId( - JNIEnv* env, - jobject obj, - jlong fake_server) { - fake_server::FakeServer* fake_server_ptr = - reinterpret_cast<fake_server::FakeServer*>(fake_server); - return base::android::ConvertUTF8ToJavaString( - env, fake_server_ptr->GetBookmarkBarFolderId()); -} - // static bool FakeServerHelperAndroid::Register(JNIEnv* env) { return RegisterNativesImpl(env); diff --git a/sync/test/fake_server/android/fake_server_helper_android.h b/sync/test/fake_server/android/fake_server_helper_android.h index 11f2234..d7449d7 100644 --- a/sync/test/fake_server/android/fake_server_helper_android.h +++ b/sync/test/fake_server/android/fake_server_helper_android.h @@ -7,7 +7,6 @@ #include <jni.h> -#include "base/android/scoped_java_ref.h" #include "base/basictypes.h" #include "sync/test/fake_server/entity_builder_factory.h" @@ -48,20 +47,6 @@ class FakeServerHelperAndroid { jstring name, jbyteArray serialized_entity_specifics); - // Injects a BookmarkEntity into |fake_server|. - void InjectBookmarkEntity(JNIEnv* env, - jobject obj, - jlong fake_server, - jstring title, - jstring url, - jstring parent_id); - - // Returns the bookmark bar folder ID. - base::android::ScopedJavaLocalRef<jstring> GetBookmarkBarFolderId( - JNIEnv* env, - jobject obj, - jlong fake_server); - private: virtual ~FakeServerHelperAndroid(); }; diff --git a/sync/test/fake_server/bookmark_entity_builder.cc b/sync/test/fake_server/bookmark_entity_builder.cc index aabc7bb..5abad58 100644 --- a/sync/test/fake_server/bookmark_entity_builder.cc +++ b/sync/test/fake_server/bookmark_entity_builder.cc @@ -45,10 +45,6 @@ BookmarkEntityBuilder::BookmarkEntityBuilder( BookmarkEntityBuilder::~BookmarkEntityBuilder() { } -void BookmarkEntityBuilder::SetParentId(const std::string& parent_id) { - parent_id_ = parent_id; -} - scoped_ptr<FakeServerEntity> BookmarkEntityBuilder::Build() { if (!url_.is_valid()) { return make_scoped_ptr<FakeServerEntity>(NULL); @@ -66,10 +62,6 @@ scoped_ptr<FakeServerEntity> BookmarkEntityBuilder::Build() { originator_client_item_id_); syncer::UniquePosition::FromInt64(0, suffix).ToProto(&unique_position); - if (parent_id_.empty()) { - parent_id_ = FakeServerEntity::CreateId(syncer::BOOKMARKS, "bookmark_bar"); - } - return make_scoped_ptr<FakeServerEntity>( new BookmarkEntity(id_, kUnusedVersion, @@ -80,7 +72,10 @@ scoped_ptr<FakeServerEntity> BookmarkEntityBuilder::Build() { entity_specifics, // TODO(pvalenzuela): Support bookmark folders. false, - parent_id_, + // TODO(pvalenzuela): Support caller specification of + // the parent bookmark folder. + FakeServerEntity::CreateId(syncer::BOOKMARKS, + "bookmark_bar"), kDefaultTime, kDefaultTime)); } diff --git a/sync/test/fake_server/bookmark_entity_builder.h b/sync/test/fake_server/bookmark_entity_builder.h index b7f897e0..62c7169 100644 --- a/sync/test/fake_server/bookmark_entity_builder.h +++ b/sync/test/fake_server/bookmark_entity_builder.h @@ -25,10 +25,6 @@ class BookmarkEntityBuilder : public EntityBuilder { ~BookmarkEntityBuilder() override; - // Sets the parend ID of the bookmark to be built. If this is not called, - // the bookmark will be included in the bookmarks bar. - void SetParentId(const std::string& parent_id); - // EntityBuilder scoped_ptr<FakeServerEntity> Build() override; @@ -37,9 +33,6 @@ class BookmarkEntityBuilder : public EntityBuilder { GURL url_; std::string originator_cache_guid_; std::string originator_client_item_id_; - - // The ID of the parent bookmark folder. - std::string parent_id_; }; } // namespace fake_server diff --git a/sync/test/fake_server/fake_server.cc b/sync/test/fake_server/fake_server.cc index dbe3105..c54a9e9 100644 --- a/sync/test/fake_server/fake_server.cc +++ b/sync/test/fake_server/fake_server.cc @@ -34,29 +34,17 @@ using syncer::GetModelType; using syncer::ModelType; using syncer::ModelTypeSet; -namespace fake_server { - -class FakeServerEntity; - -namespace { - // The default store birthday value. static const char kDefaultStoreBirthday[] = "1234567890"; // The default keystore key. static const char kDefaultKeystoreKey[] = "1111111111111111"; -// Properties of the bookmark bar permanent folder. -static const char kBookmarkBarFolderServerTag[] = "bookmark_bar"; -static const char kBookmarkBarFolderName[] = "Bookmark Bar"; +namespace fake_server { -// Properties of the other bookmarks permanent folder. -static const char kOtherBookmarksFolderServerTag[] = "other_bookmarks"; -static const char kOtherBookmarksFolderName[] = "Other Bookmarks"; +class FakeServerEntity; -// Properties of the synced bookmarks permanent folder. -static const char kSyncedBookmarksFolderServerTag[] = "synced_bookmarks"; -static const char kSyncedBookmarksFolderName[] = "Synced Bookmarks"; +namespace { // A filter used during GetUpdates calls to determine what information to // send back to the client. There is a 1:1 correspondence between any given @@ -179,8 +167,8 @@ FakeServer::~FakeServer() { STLDeleteContainerPairSecondPointers(entities_.begin(), entities_.end()); } -bool FakeServer::CreatePermanentBookmarkFolder(const std::string& server_tag, - const std::string& name) { +bool FakeServer::CreatePermanentBookmarkFolder(const char* server_tag, + const char* name) { FakeServerEntity* entity = PermanentEntity::Create(syncer::BOOKMARKS, server_tag, name, ModelTypeToRootTag(syncer::BOOKMARKS)); @@ -203,11 +191,9 @@ bool FakeServer::CreateDefaultPermanentItems() { SaveEntity(top_level_entity); if (model_type == syncer::BOOKMARKS) { - if (!CreatePermanentBookmarkFolder(kBookmarkBarFolderServerTag, - kBookmarkBarFolderName)) + if (!CreatePermanentBookmarkFolder("bookmark_bar", "Bookmark Bar")) return false; - if (!CreatePermanentBookmarkFolder(kOtherBookmarksFolderServerTag, - kOtherBookmarksFolderName)) + if (!CreatePermanentBookmarkFolder("other_bookmarks", "Other Bookmarks")) return false; } } @@ -215,6 +201,21 @@ bool FakeServer::CreateDefaultPermanentItems() { return true; } +bool FakeServer::CreateMobileBookmarksPermanentItem() { + // This folder is called "Synced Bookmarks" by sync and is renamed + // "Mobile Bookmarks" by the mobile client UIs. + FakeServerEntity* mobile_bookmarks_entity = + PermanentEntity::Create(syncer::BOOKMARKS, + "synced_bookmarks", + "Synced Bookmarks", + ModelTypeToRootTag(syncer::BOOKMARKS)); + if (mobile_bookmarks_entity == NULL) { + return false; + } + SaveEntity(mobile_bookmarks_entity); + return true; +} + void FakeServer::SaveEntity(FakeServerEntity* entity) { delete entities_[entity->GetId()]; entity->SetVersion(++version_); @@ -290,11 +291,8 @@ bool FakeServer::HandleGetUpdatesRequest( scoped_ptr<UpdateSieve> sieve = UpdateSieve::Create(get_updates); - // This folder is called "Synced Bookmarks" by sync and is renamed - // "Mobile Bookmarks" by the mobile client UIs. if (get_updates.create_mobile_bookmarks_folder() && - !CreatePermanentBookmarkFolder(kSyncedBookmarksFolderServerTag, - kSyncedBookmarksFolderName)) { + !CreateMobileBookmarksPermanentItem()) { return false; } @@ -603,18 +601,4 @@ void FakeServer::DisableNetwork() { network_enabled_ = false; } -std::string FakeServer::GetBookmarkBarFolderId() const { - for (EntityMap::const_iterator it = entities_.begin(); it != entities_.end(); - ++it) { - FakeServerEntity* entity = it->second; - if (entity->GetName() == kBookmarkBarFolderName && - entity->IsFolder() && - entity->GetModelType() == syncer::BOOKMARKS) { - return entity->GetId(); - } - } - NOTREACHED() << "Bookmark Bar entity not found."; - return ""; -} - } // namespace fake_server diff --git a/sync/test/fake_server/fake_server.h b/sync/test/fake_server/fake_server.h index ecc8bd7..78edd9a 100644 --- a/sync/test/fake_server/fake_server.h +++ b/sync/test/fake_server/fake_server.h @@ -116,9 +116,6 @@ class FakeServer { // This can be used to trigger exponential backoff in the client. void DisableNetwork(); - // Returns the entity ID of the Bookmark Bar folder. - std::string GetBookmarkBarFolderId() const; - private: typedef std::map<std::string, FakeServerEntity*> EntityMap; @@ -131,13 +128,14 @@ class FakeServer { const std::string& invalidator_client_id, sync_pb::CommitResponse* response); - // Creates and saves a permanent folder for Bookmarks (e.g., Bookmark Bar). - bool CreatePermanentBookmarkFolder(const std::string& server_tag, - const std::string& name); + bool CreatePermanentBookmarkFolder(const char* server_tag, const char* name); // Inserts the default permanent items in |entities_|. bool CreateDefaultPermanentItems(); + // Inserts the mobile bookmarks folder entity in |entities_|. + bool CreateMobileBookmarksPermanentItem(); + // Saves a |entity| to |entities_|. void SaveEntity(FakeServerEntity* entity); diff --git a/tools/mb/mb_config.pyl b/tools/mb/mb_config.pyl index 53bbf03..7af21da 100644 --- a/tools/mb/mb_config.pyl +++ b/tools/mb/mb_config.pyl @@ -22,10 +22,8 @@ 'gn_release_bot': ['gn', 'release_bot'], 'gn_release_bot_x86': ['gn', 'release_bot', 'x86'], 'gn_release_trybot': ['gn', 'release_trybot'], - 'gn_release_trybot_x86': ['gn', 'release_trybot', 'x86'], 'gn_debug_bot': ['gn', 'debug_bot'], - 'gn_debug_static_bot': ['gn', 'debug_static_bot'], - 'gn_debug_static_bot_x86': ['gn', 'debug_static_bot', 'x86'], + 'gn_debug_bot_win_x86': ['gn', 'debug_static_bot', 'x86'], 'gyp_release_bot': ['gyp', 'release_bot'], }, @@ -147,11 +145,11 @@ }, 'chromium.mac': { 'Mac GN': 'gn_release_bot', - 'Mac GN (dbg)': 'gn_debug_static_bot', + 'Mac GN (dbg)': 'gn_debug_bot', }, 'chromium.win': { 'Win8 GN': 'gn_release_bot_x86', - 'Win8 GN (dbg)': 'gn_debug_static_bot_x86', + 'Win8 GN (dbg)': 'gn_debug_bot_win_x86', }, 'chromium.webkit': { 'Android GN': 'gn_release_bot', @@ -174,13 +172,13 @@ 'linux_chromium_gn_upload_x86': 'gn_release_bot_x86', }, 'tryserver.chromium.mac': { - 'mac_chromium_gn_dbg': 'gn_debug_static_bot', + 'mac_chromium_gn_dbg': 'gn_debug_bot', 'mac_chromium_gn_rel': 'gn_release_trybot', 'mac_chromium_gn_upload': 'gn_release_bot', }, 'tryserver.chromium.win': { - 'win8_chromium_gn_dbg': 'gn_debug_static_bot_x86', - 'win8_chromium_gn_rel': 'gn_release_trybot_x86', + 'win8_chromium_gn_dbg': 'gn_debug_bot', + 'win8_chromium_gn_rel': 'gn_release_trybot', 'win8_chromium_gn_upload': 'gn_release_bot', }, }, diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index eee68bd..f53c021 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -24274,15 +24274,6 @@ Therefore, the affected-histogram name has to have at least one dot in it. </summary> </histogram> -<histogram name="Notifications.AuthorDataSizeKB" units="KB"> - <owner>peter@chromium.org</owner> - <summary> - The size, in kilobytes, of the author-provided data associated with a Web - Notification. Recorded when a persistent Web Notification is being shown by - the developer. - </summary> -</histogram> - <histogram name="Notifications.Database.DeleteResult" enum="NotificationDatabaseStatus"> <owner>peter@chromium.org</owner> diff --git a/tools/run-bisect-perf-regression.py b/tools/run-bisect-perf-regression.py index a9c7e54..dda359b 100755 --- a/tools/run-bisect-perf-regression.py +++ b/tools/run-bisect-perf-regression.py @@ -37,7 +37,6 @@ WEBKIT_RUN_TEST_CONFIG_PATH = os.path.join( BISECT_SCRIPT_DIR = os.path.join(SCRIPT_DIR, 'auto_bisect') PERF_BENCHMARKS_PATH = 'tools/perf/benchmarks' -PERF_MEASUREMENTS_PATH = 'tools/perf/measurements' BUILDBOT_BUILDERNAME = 'BUILDBOT_BUILDERNAME' BENCHMARKS_JSON_FILE = 'benchmarks.json' @@ -635,8 +634,7 @@ def _GetAffectedBenchmarkModuleNames(): all_affected_files = _GetModifiedFilesFromPatch() modified_benchmarks = [] for affected_file in all_affected_files: - if (affected_file.startswith(PERF_BENCHMARKS_PATH) or - affected_file.startswith(PERF_MEASUREMENTS_PATH)): + if affected_file.startswith(PERF_BENCHMARKS_PATH): benchmark = os.path.basename(os.path.splitext(affected_file)[0]) modified_benchmarks.append(benchmark) return modified_benchmarks diff --git a/ui/compositor/layer.cc b/ui/compositor/layer.cc index 3e61d40..34c11e3 100644 --- a/ui/compositor/layer.cc +++ b/ui/compositor/layer.cc @@ -751,11 +751,11 @@ void Layer::PaintContents( delegate_->OnPaintLayer(PaintContext(canvas.get(), clip)); } -void Layer::PaintContentsToDisplayList( - cc::DisplayItemList* display_list, +scoped_refptr<cc::DisplayItemList> Layer::PaintContentsToDisplayList( const gfx::Rect& clip, ContentLayerClient::PaintingControlSetting painting_control) { TRACE_EVENT1("ui", "Layer::PaintContentsToDisplayList", "name", name_); + scoped_refptr<cc::DisplayItemList> list = cc::DisplayItemList::Create(); if (delegate_) { // TODO(danakj): Save the invalidation on the layer and pass that down // instead of the |clip| here. That will break everything until View @@ -763,8 +763,9 @@ void Layer::PaintContentsToDisplayList( gfx::Rect invalidation = clip; DCHECK(clip.Contains(invalidation)); delegate_->OnPaintLayer( - PaintContext(display_list, device_scale_factor_, clip, invalidation)); + PaintContext(list.get(), device_scale_factor_, clip, invalidation)); } + return list; } bool Layer::FillsBoundsCompletely() const { return fills_bounds_completely_; } diff --git a/ui/compositor/layer.h b/ui/compositor/layer.h index 381e982..3dbd173 100644 --- a/ui/compositor/layer.h +++ b/ui/compositor/layer.h @@ -342,8 +342,7 @@ class COMPOSITOR_EXPORT Layer SkCanvas* canvas, const gfx::Rect& clip, ContentLayerClient::PaintingControlSetting painting_control) override; - void PaintContentsToDisplayList( - cc::DisplayItemList* display_list, + scoped_refptr<cc::DisplayItemList> PaintContentsToDisplayList( const gfx::Rect& clip, ContentLayerClient::PaintingControlSetting painting_control) override; bool FillsBoundsCompletely() const override; diff --git a/ui/file_manager/file_manager/foreground/css/cws_widget_container.css b/ui/file_manager/file_manager/foreground/css/cws_widget_container.css index 8e4c5812..37e16eb 100644 --- a/ui/file_manager/file_manager/foreground/css/cws_widget_container.css +++ b/ui/file_manager/file_manager/foreground/css/cws_widget_container.css @@ -63,20 +63,3 @@ flex: 1; line-height: 16px; } - -.cr-dialog-frame.cws-widget-error-dialog-frame { - width: 300px; -} - -.cws-widget-error-dialog-frame .cws-widget-error-dialog-img { - background-image: -webkit-image-set( - url(chrome://theme/IDR_ERROR_NETWORK_GENERIC) 1x, - url(chrome://theme/IDR_ERROR_NETWORK_GENERIC@2x) 2x); - background-position: center; - background-repeat: no-repeat; - height: 40px; -} - -.cws-widget-error-dialog-text { - text-align: center; -} diff --git a/ui/file_manager/file_manager/foreground/js/compiled_resources.gyp b/ui/file_manager/file_manager/foreground/js/compiled_resources.gyp index b1a5b85..8f99f24 100644 --- a/ui/file_manager/file_manager/foreground/js/compiled_resources.gyp +++ b/ui/file_manager/file_manager/foreground/js/compiled_resources.gyp @@ -114,7 +114,6 @@ './list_thumbnail_loader.js', './ui/banners.js', './ui/conflict_dialog.js', - './ui/cws_widget_container_error_dialog.js', './ui/default_action_dialog.js', './ui/dialog_footer.js', './ui/directory_tree.js', diff --git a/ui/file_manager/file_manager/foreground/js/cws_widget_container.js b/ui/file_manager/file_manager/foreground/js/cws_widget_container.js index 45548c1..46183c7 100644 --- a/ui/file_manager/file_manager/foreground/js/cws_widget_container.js +++ b/ui/file_manager/file_manager/foreground/js/cws_widget_container.js @@ -67,8 +67,6 @@ function CWSWidgetContainer(document, parentNode, state) { this.webviewContainer_.style.height = WEBVIEW_HEIGHT + 'px'; parentNode.appendChild(this.webviewContainer_); - parentNode.classList.add('cws-widget-container-root'); - /** * Element showing spinner layout in place of Web Store widget. * @type {!Element} @@ -192,13 +190,6 @@ function CWSWidgetContainer(document, parentNode, state) { * @private */ this.tokenGetter_ = this.createTokenGetter_(); - - /** - * Dialog to be shown when an installation attempt fails. - * @type {CWSWidgetContainerErrorDialog} - * @private - */ - this.errorDialog_ = new CWSWidgetContainerErrorDialog(parentNode); } /** @@ -528,7 +519,8 @@ CWSWidgetContainer.prototype.onItemInstalled_ = function(result, error) { case AppInstaller.Result.ERROR: CWSWidgetContainer.Metrics.recordInstall( CWSWidgetContainer.Metrics.INSTALL.FAILED); - this.errorDialog_.show( + // TODO(tbarzic): Remove dialog showing call from this class. + fileManager.ui.errorDialog.show( str('SUGGEST_DIALOG_INSTALLATION_FAILED'), null, null, @@ -652,9 +644,6 @@ CWSWidgetContainer.prototype.reset_ = function () { this.appInstaller_.cancel(); this.options_ = null; - - if (this.errorDialog_.shown()) - this.errorDialog_.hide(); }; /** diff --git a/ui/file_manager/file_manager/foreground/js/main_scripts.js b/ui/file_manager/file_manager/foreground/js/main_scripts.js index c078f95..04b8628 100644 --- a/ui/file_manager/file_manager/foreground/js/main_scripts.js +++ b/ui/file_manager/file_manager/foreground/js/main_scripts.js @@ -132,7 +132,6 @@ //<include src="list_thumbnail_loader.js"> //<include src="ui/banners.js" > //<include src="ui/conflict_dialog.js"> -//<include src="ui/cws_widget_container_error_dialog.js"> //<include src="ui/default_action_dialog.js"> //<include src="ui/dialog_footer.js"> //<include src="ui/directory_tree.js"> diff --git a/ui/file_manager/file_manager/foreground/js/ui/cws_widget_container_error_dialog.js b/ui/file_manager/file_manager/foreground/js/ui/cws_widget_container_error_dialog.js deleted file mode 100644 index 28efbb4..0000000 --- a/ui/file_manager/file_manager/foreground/js/ui/cws_widget_container_error_dialog.js +++ /dev/null @@ -1,58 +0,0 @@ -// Copyright 2015 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -/** - * @param {HTMLElement} parentNode Node to be parent for this dialog. - * @constructor - * @extends {cr.ui.dialogs.BaseDialog} - */ -function CWSWidgetContainerErrorDialog(parentNode) { - cr.ui.dialogs.BaseDialog.call(this, parentNode); -} - -CWSWidgetContainerErrorDialog.prototype = { - __proto__: cr.ui.dialogs.BaseDialog.prototype -}; - -/** - * Whether the dialog is showm. - * @return {boolean} - */ -CWSWidgetContainerErrorDialog.prototype.shown = function() { - return this.container_.classList.contains('shown'); -}; - -/** - * One-time initialization of DOM. - * @private - */ -CWSWidgetContainerErrorDialog.prototype.initDom_ = function() { - cr.ui.dialogs.BaseDialog.prototype.initDom_.call(this); - this.frame_.classList.add('cws-widget-error-dialog-frame'); - var img = this.document_.createElement('div'); - img.className = 'cws-widget-error-dialog-img'; - this.frame_.insertBefore(img, this.text_); - - this.title_.hidden = true; - this.closeButton_.hidden = true; - this.cancelButton_.hidden = true; - this.text_.classList.add('cws-widget-error-dialog-text'); - - // Don't allow OK button to lose focus, in order to prevent webview content - // from stealing focus. - // BaseDialog keeps focus by removing all other focusable elements from tab - // order (by setting their tabIndex to -1). This doesn't work for webviews - // because the webview embedder cannot access the webview DOM tree, and thus - // fails to remove elements in the webview from tab order. - this.okButton_.addEventListener('blur', this.refocusOkButton_.bind(this)); -}; - -/** - * Focuses OK button. - * @private - */ -CWSWidgetContainerErrorDialog.prototype.refocusOkButton_ = function() { - if (this.shown()) - this.okButton_.focus(); -}; diff --git a/ui/file_manager/file_manager/main.html b/ui/file_manager/file_manager/main.html index a4ae31d..4cc1490 100644 --- a/ui/file_manager/file_manager/main.html +++ b/ui/file_manager/file_manager/main.html @@ -155,7 +155,6 @@ <script src="foreground/js/list_thumbnail_loader.js"></script> <script src="foreground/js/ui/banners.js"></script> <script src="foreground/js/ui/conflict_dialog.js"></script> - <script src="foreground/js/ui/cws_widget_container_error_dialog.js"></script> <script src="foreground/js/ui/default_action_dialog.js"></script> <script src="foreground/js/ui/dialog_footer.js"></script> <script src="foreground/js/ui/directory_tree.js"></script> |