diff options
163 files changed, 1784 insertions, 1990 deletions
@@ -469,7 +469,6 @@ 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': 'eddab4425614e49146f904f00da4a664ba4b581b', + 'pdfium_revision': 'b3300162a1ebacc973ff9793029caf4db9a4f5e5', # 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,6 +274,9 @@ '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/|'\ @@ -925,6 +928,7 @@ '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 07659d7c..dbd4465 100644 --- a/ash/content/display/screen_orientation_controller_chromeos.cc +++ b/ash/content/display/screen_orientation_controller_chromeos.cc @@ -196,10 +196,11 @@ 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 = - Shell::GetInstance() - ->display_manager() - ->GetDisplayInfo(gfx::Display::InternalDisplayId()) + display_manager->GetDisplayInfo(gfx::Display::InternalDisplayId()) .rotation(); if (user_rotation != current_rotation_) { // A user may change other display configuration settings. When the user @@ -212,11 +213,14 @@ void ScreenOrientationController::OnDisplayConfigurationChanged() { void ScreenOrientationController::OnMaximizeModeStarted() { DisplayManager* display_manager = Shell::GetInstance()->display_manager(); - if (!display_manager->HasInternalDisplay()) - return; - current_rotation_ = user_rotation_ = - display_manager->GetDisplayInfo(gfx::Display::InternalDisplayId()) - .rotation(); + // 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 (!rotation_locked_) LoadDisplayRotationProperties(); chromeos::AccelerometerReader::GetInstance()->AddObserver(this); @@ -224,8 +228,6 @@ 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 ca02ca3..342e958 100644 --- a/ash/content/display/screen_orientation_controller_chromeos_unittest.cc +++ b/ash/content/display/screen_orientation_controller_chromeos_unittest.cc @@ -589,4 +589,23 @@ 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 6630321..1e927fb 100755 --- a/build/android/buildbot/bb_host_steps.py +++ b/build/android/buildbot/bb_host_steps.py @@ -4,6 +4,7 @@ # found in the LICENSE file. import os +import json import sys import bb_utils @@ -85,7 +86,9 @@ 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)] + args) + '-w', os.path.join(constants.DIR_SOURCE_ROOT, os.pardir), + '--build-properties=%s' % json.dumps(options.build_properties)] + + args) def GetHostStepCmds(): diff --git a/build/gn_migration.gypi b/build/gn_migration.gypi index 1dacbb5..ad6560c 100644 --- a/build/gn_migration.gypi +++ b/build/gn_migration.gypi @@ -281,7 +281,6 @@ ['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 e335ba3..e69abf6 100644 --- a/cc/blink/web_compositor_support_impl.cc +++ b/cc/blink/web_compositor_support_impl.cc @@ -93,10 +93,6 @@ 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 4b41691..b924be9 100644 --- a/cc/blink/web_compositor_support_impl.h +++ b/cc/blink/web_compositor_support_impl.h @@ -42,7 +42,6 @@ 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 92242230..90d7f9b 100644 --- a/cc/blink/web_content_layer_impl.cc +++ b/cc/blink/web_content_layer_impl.cc @@ -72,16 +72,15 @@ void WebContentLayerImpl::PaintContents( client_->paintContents(canvas, clip, PaintingControlToWeb(painting_control)); } -scoped_refptr<cc::DisplayItemList> -WebContentLayerImpl::PaintContentsToDisplayList( +void WebContentLayerImpl::PaintContentsToDisplayList( + cc::DisplayItemList* display_list, const gfx::Rect& clip, cc::ContentLayerClient::PaintingControlSetting painting_control) { if (!client_) - return cc::DisplayItemList::Create(); + return; - WebDisplayItemListImpl list; + WebDisplayItemListImpl list(display_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 3e7b55c..e7b04b0 100644 --- a/cc/blink/web_content_layer_impl.h +++ b/cc/blink/web_content_layer_impl.h @@ -39,7 +39,8 @@ class WebContentLayerImpl : public blink::WebContentLayer, void PaintContents(SkCanvas* canvas, const gfx::Rect& clip, PaintingControlSetting painting_control) override; - scoped_refptr<cc::DisplayItemList> PaintContentsToDisplayList( + void PaintContentsToDisplayList( + cc::DisplayItemList* display_list, 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 a253beb..2bad804 100644 --- a/cc/blink/web_display_item_list_impl.cc +++ b/cc/blink/web_display_item_list_impl.cc @@ -25,12 +25,9 @@ namespace cc_blink { -WebDisplayItemListImpl::WebDisplayItemListImpl() - : display_item_list_(cc::DisplayItemList::Create()) { -} - -scoped_refptr<cc::DisplayItemList> WebDisplayItemListImpl::ToDisplayItemList() { - return display_item_list_; +WebDisplayItemListImpl::WebDisplayItemListImpl( + cc::DisplayItemList* display_list) + : display_item_list_(display_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 abaf221..ff94ee3 100644 --- a/cc/blink/web_display_item_list_impl.h +++ b/cc/blink/web_display_item_list_impl.h @@ -31,11 +31,9 @@ namespace cc_blink { class WebDisplayItemListImpl : public blink::WebDisplayItemList { public: - CC_BLINK_EXPORT WebDisplayItemListImpl(); + CC_BLINK_EXPORT WebDisplayItemListImpl(cc::DisplayItemList* display_list); virtual ~WebDisplayItemListImpl(); - scoped_refptr<cc::DisplayItemList> ToDisplayItemList(); - // blink::WebDisplayItemList implementation. virtual void appendDrawingItem(const SkPicture*); virtual void appendClipItem( @@ -63,7 +61,7 @@ class WebDisplayItemListImpl : public blink::WebDisplayItemList { virtual void appendEndScrollItem(); private: - scoped_refptr<cc::DisplayItemList> display_item_list_; + 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 ab05f30..f9a48c9 100644 --- a/cc/debug/rasterize_and_record_benchmark.cc +++ b/cc/debug/rasterize_and_record_benchmark.cc @@ -206,8 +206,11 @@ void RasterizeAndRecordBenchmark::RunOnDisplayListLayer( kTimeCheckInterval); do { - display_list = painter->PaintContentsToDisplayList(visible_layer_rect, - painting_control); + 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->CreateAndCacheSkPicture(); if (memory_used) { diff --git a/cc/layers/content_layer_client.h b/cc/layers/content_layer_client.h index 4f9c754..3a82136 100644 --- a/cc/layers/content_layer_client.h +++ b/cc/layers/content_layer_client.h @@ -29,7 +29,8 @@ class CC_EXPORT ContentLayerClient { const gfx::Rect& clip, PaintingControlSetting painting_status) = 0; - virtual scoped_refptr<DisplayItemList> PaintContentsToDisplayList( + virtual void PaintContentsToDisplayList( + DisplayItemList* display_list, 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 b972ec1..2d338f0 100644 --- a/cc/layers/picture_image_layer.cc +++ b/cc/layers/picture_image_layer.cc @@ -63,19 +63,17 @@ void PictureImageLayer::PaintContents( canvas->drawBitmap(bitmap_, 0, 0); } -scoped_refptr<DisplayItemList> PictureImageLayer::PaintContentsToDisplayList( +void PictureImageLayer::PaintContentsToDisplayList( + DisplayItemList* display_list, 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_item_list->AppendItem(DrawingDisplayItem::Create(picture)); - return display_item_list; + display_list->AppendItem(DrawingDisplayItem::Create(picture)); } bool PictureImageLayer::FillsBoundsCompletely() const { diff --git a/cc/layers/picture_image_layer.h b/cc/layers/picture_image_layer.h index 8d40550..a12a1b9 100644 --- a/cc/layers/picture_image_layer.h +++ b/cc/layers/picture_image_layer.h @@ -27,7 +27,8 @@ class CC_EXPORT PictureImageLayer : public PictureLayer, ContentLayerClient { SkCanvas* canvas, const gfx::Rect& clip, ContentLayerClient::PaintingControlSetting painting_control) override; - scoped_refptr<DisplayItemList> PaintContentsToDisplayList( + void PaintContentsToDisplayList( + DisplayItemList* display_list, 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 5b9375d..cd95c9f 100644 --- a/cc/layers/picture_image_layer_unittest.cc +++ b/cc/layers/picture_image_layer_unittest.cc @@ -33,9 +33,44 @@ 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 = - layer->PaintContentsToDisplayList( - layer_rect, ContentLayerClient::PAINTING_BEHAVIOR_NORMAL); + 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(); 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 2f64693..917a36f 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 {} - scoped_refptr<DisplayItemList> PaintContentsToDisplayList( + void PaintContentsToDisplayList( + DisplayItemList* display_list, 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 45e6c411..ff632f6 100644 --- a/cc/layers/video_frame_provider.h +++ b/cc/layers/video_frame_provider.h @@ -6,6 +6,7 @@ #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 { @@ -14,27 +15,39 @@ class VideoFrame; namespace cc { -// 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. +// 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). class CC_EXPORT VideoFrameProvider { public: - virtual ~VideoFrameProvider() {} - class CC_EXPORT Client { public: - // Provider will call this method to tell the client to stop using it. + // The 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 provider's client that a call to GetCurrentFrame() will - // return new data. + // 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 virtual void DidReceiveFrame() = 0; - // Notifies the provider's client of a new UV transform matrix to be used. + // Notifies the client of a new UV transform matrix to be used. virtual void DidUpdateMatrix(const float* matrix) = 0; protected: @@ -45,18 +58,33 @@ class CC_EXPORT VideoFrameProvider { // that the provider is not destroyed before this call returns. virtual void SetVideoFrameProviderClient(Client* client) = 0; - // 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. + // 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 virtual scoped_refptr<media::VideoFrame> GetCurrentFrame() = 0; - // 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; + // 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() {} }; } // namespace cc diff --git a/cc/layers/video_frame_provider_client_impl.cc b/cc/layers/video_frame_provider_client_impl.cc index 68e10dc..339d4fc 100644 --- a/cc/layers/video_frame_provider_client_impl.cc +++ b/cc/layers/video_frame_provider_client_impl.cc @@ -78,11 +78,10 @@ VideoFrameProviderClientImpl::AcquireLockAndCurrentFrame() { return provider_->GetCurrentFrame(); } -void VideoFrameProviderClientImpl::PutCurrentFrame( - const scoped_refptr<media::VideoFrame>& frame) { +void VideoFrameProviderClientImpl::PutCurrentFrame() { DCHECK(thread_checker_.CalledOnValidThread()); provider_lock_.AssertAcquired(); - provider_->PutCurrentFrame(frame); + provider_->PutCurrentFrame(); } void VideoFrameProviderClientImpl::ReleaseLock() { @@ -104,6 +103,16 @@ 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 2688ad2..ba2eb73 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(const scoped_refptr<media::VideoFrame>& frame); + void PutCurrentFrame(); void ReleaseLock(); const gfx::Transform& StreamTextureMatrix() const; @@ -46,6 +46,8 @@ 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 3ca0978..0e9add0 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(frame_); + provider_client_impl_->PutCurrentFrame(); frame_ = nullptr; provider_client_impl_->ReleaseLock(); diff --git a/cc/resources/display_item.cc b/cc/resources/display_item.cc index 52bdec1..33069a53 100644 --- a/cc/resources/display_item.cc +++ b/cc/resources/display_item.cc @@ -9,8 +9,4 @@ 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 ba4e733..d065958 100644 --- a/cc/resources/display_item.h +++ b/cc/resources/display_item.h @@ -21,7 +21,6 @@ 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 b541c5f..0b2e12e 100644 --- a/cc/resources/display_item_list.cc +++ b/cc/resources/display_item_list.cc @@ -20,12 +20,36 @@ namespace cc { -DisplayItemList::DisplayItemList() - : is_suitable_for_gpu_rasterization_(true), approximate_op_count_(0) { +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; + } } -scoped_refptr<DisplayItemList> DisplayItemList::Create() { - return make_scoped_refptr(new DisplayItemList()); +scoped_refptr<DisplayItemList> DisplayItemList::Create( + gfx::Rect layer_rect, + bool use_cached_picture) { + return make_scoped_refptr( + new DisplayItemList(layer_rect, use_cached_picture)); } DisplayItemList::~DisplayItemList() { @@ -34,7 +58,7 @@ DisplayItemList::~DisplayItemList() { void DisplayItemList::Raster(SkCanvas* canvas, SkDrawPictureCallback* callback, float contents_scale) const { - if (!picture_) { + if (!use_cached_picture_) { canvas->save(); canvas->scale(contents_scale, contents_scale); for (size_t i = 0; i < items_.size(); ++i) { @@ -62,25 +86,25 @@ void DisplayItemList::Raster(SkCanvas* canvas, } void DisplayItemList::CreateAndCacheSkPicture() { - // 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()); + // Convert to an SkPicture for faster rasterization. + DCHECK(use_cached_picture_); + 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(); - items_.push_back(item.Pass()); + + if (use_cached_picture_) { + DCHECK(canvas_); + item->Raster(canvas_.get(), NULL); + } + + if (retain_individual_display_items_) + items_.push_back(item.Pass()); } bool DisplayItemList::IsSuitableForGpuRasterization() const { @@ -126,8 +150,7 @@ 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_)); - for (size_t i = 0; i < items_.size(); ++i) - items_[i]->RasterForTracing(canvas); + Raster(canvas, NULL, 1.f); 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 f490552..0a5d468 100644 --- a/cc/resources/display_item_list.h +++ b/cc/resources/display_item_list.h @@ -18,13 +18,15 @@ class SkCanvas; class SkDrawPictureCallback; +class SkPictureRecorder; namespace cc { class CC_EXPORT DisplayItemList : public base::RefCountedThreadSafe<DisplayItemList> { public: - static scoped_refptr<DisplayItemList> Create(); + static scoped_refptr<DisplayItemList> Create(gfx::Rect layer_rect, + bool use_cached_picture); void Raster(SkCanvas* canvas, SkDrawPictureCallback* callback, @@ -32,9 +34,6 @@ 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; @@ -48,11 +47,16 @@ class CC_EXPORT DisplayItemList void GatherPixelRefs(const gfx::Size& grid_cell_size); private: - DisplayItemList(); + DisplayItemList(gfx::Rect layer_rect, bool use_cached_picture); ~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 b53b84a..290b25b 100644 --- a/cc/resources/display_item_list_unittest.cc +++ b/cc/resources/display_item_list_unittest.cc @@ -36,7 +36,9 @@ TEST(DisplayItemListTest, SingleDrawingItem) { SkPaint red_paint; red_paint.setColor(SK_ColorRED); unsigned char pixels[4 * 100 * 100] = {0}; - scoped_refptr<DisplayItemList> list = DisplayItemList::Create(); + const bool use_cached_picture = true; + scoped_refptr<DisplayItemList> list = + DisplayItemList::Create(layer_rect, use_cached_picture); gfx::PointF offset(8.f, 9.f); gfx::RectF recording_rect(offset, layer_rect.size()); @@ -47,6 +49,7 @@ 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; @@ -76,7 +79,9 @@ TEST(DisplayItemListTest, ClipItem) { SkPaint red_paint; red_paint.setColor(SK_ColorRED); unsigned char pixels[4 * 100 * 100] = {0}; - scoped_refptr<DisplayItemList> list = DisplayItemList::Create(); + const bool use_cached_picture = true; + scoped_refptr<DisplayItemList> list = + DisplayItemList::Create(layer_rect, use_cached_picture); gfx::PointF first_offset(8.f, 9.f); gfx::RectF first_recording_rect(first_offset, layer_rect.size()); @@ -100,6 +105,7 @@ TEST(DisplayItemListTest, ClipItem) { list->AppendItem(DrawingDisplayItem::Create(picture)); list->AppendItem(EndClipDisplayItem::Create()); + list->CreateAndCacheSkPicture(); DrawDisplayList(pixels, layer_rect, list); @@ -131,7 +137,9 @@ TEST(DisplayItemListTest, TransformItem) { SkPaint red_paint; red_paint.setColor(SK_ColorRED); unsigned char pixels[4 * 100 * 100] = {0}; - scoped_refptr<DisplayItemList> list = DisplayItemList::Create(); + const bool use_cached_picture = true; + scoped_refptr<DisplayItemList> list = + DisplayItemList::Create(layer_rect, use_cached_picture); gfx::PointF first_offset(8.f, 9.f); gfx::RectF first_recording_rect(first_offset, layer_rect.size()); @@ -156,6 +164,7 @@ TEST(DisplayItemListTest, TransformItem) { list->AppendItem(DrawingDisplayItem::Create(picture)); list->AppendItem(EndTransformDisplayItem::Create()); + list->CreateAndCacheSkPicture(); DrawDisplayList(pixels, layer_rect, list); @@ -177,11 +186,13 @@ TEST(DisplayItemListTest, TransformItem) { EXPECT_EQ(0, memcmp(pixels, expected_pixels, 4 * 100 * 100)); } -TEST(DisplayItemList, FilterItem) { +TEST(DisplayItemListTest, FilterItem) { gfx::Rect layer_rect(100, 100); FilterOperations filters; unsigned char pixels[4 * 100 * 100] = {0}; - scoped_refptr<DisplayItemList> list = DisplayItemList::Create(); + const bool use_cached_picture = true; + scoped_refptr<DisplayItemList> list = + DisplayItemList::Create(layer_rect, use_cached_picture); SkBitmap source_bitmap; source_bitmap.allocN32Pixels(50, 50); @@ -206,6 +217,7 @@ TEST(DisplayItemList, 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); @@ -236,8 +248,9 @@ TEST(DisplayItemListTest, CompactingItems) { gfx::PointF offset(8.f, 9.f); gfx::RectF recording_rect(offset, layer_rect.size()); - scoped_refptr<DisplayItemList> list = DisplayItemList::Create(); - list->set_layer_rect(ToEnclosingRect(recording_rect)); + bool use_cached_picture = false; + scoped_refptr<DisplayItemList> list_without_caching = + DisplayItemList::Create(layer_rect, use_cached_picture); canvas = skia::SharePtr( recorder.beginRecording(gfx::RectFToSkRect(recording_rect))); @@ -245,13 +258,16 @@ 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->AppendItem(DrawingDisplayItem::Create(picture)); - DrawDisplayList(pixels, layer_rect, list); - - list->CreateAndCacheSkPicture(); + list_without_caching->AppendItem(DrawingDisplayItem::Create(picture)); + DrawDisplayList(pixels, layer_rect, list_without_caching); unsigned char expected_pixels[4 * 100 * 100] = {0}; - DrawDisplayList(expected_pixels, layer_rect, list); + 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); 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 7ada580..701db9b 100644 --- a/cc/resources/display_list_recording_source.cc +++ b/cc/resources/display_list_recording_source.cc @@ -119,17 +119,18 @@ bool DisplayListRecordingSource::UpdateAndExpandInvalidation( } } for (int i = 0; i < repeat_count; ++i) { - display_list_ = painter->PaintContentsToDisplayList(recorded_viewport_, - painting_control); + 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_->set_layer_rect(recorded_viewport_); + display_list_->CreateAndCacheSkPicture(); + 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 648f9de..91ab3fb 100644 --- a/cc/resources/drawing_display_item.cc +++ b/cc/resources/drawing_display_item.cc @@ -34,17 +34,6 @@ 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 b45a039..a3eef77 100644 --- a/cc/resources/drawing_display_item.h +++ b/cc/resources/drawing_display_item.h @@ -27,7 +27,6 @@ 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 296c891..c21d87b 100644 --- a/cc/test/fake_content_layer_client.cc +++ b/cc/test/fake_content_layer_client.cc @@ -71,15 +71,15 @@ void FakeContentLayerClient::PaintContents( } } -scoped_refptr<DisplayItemList> -FakeContentLayerClient::PaintContentsToDisplayList( +void FakeContentLayerClient::PaintContentsToDisplayList( + DisplayItemList* display_list, const gfx::Rect& clip, PaintingControlSetting painting_control) { SkPictureRecorder recorder; skia::RefPtr<SkCanvas> canvas; skia::RefPtr<SkPicture> picture; - scoped_refptr<DisplayItemList> list = DisplayItemList::Create(); - list->AppendItem(ClipDisplayItem::Create(clip, std::vector<SkRRect>())); + display_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 @@ FakeContentLayerClient::PaintContentsToDisplayList( canvas->drawRectCoords(draw_rect.x(), draw_rect.y(), draw_rect.width(), draw_rect.height(), paint); picture = skia::AdoptRef(recorder.endRecordingAsPicture()); - list->AppendItem(DrawingDisplayItem::Create(picture)); + display_list->AppendItem(DrawingDisplayItem::Create(picture)); } for (BitmapVector::const_iterator it = draw_bitmaps_.begin(); it != draw_bitmaps_.end(); ++it) { if (!it->transform.IsIdentity()) { - list->AppendItem(TransformDisplayItem::Create(it->transform)); + display_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()); - list->AppendItem(DrawingDisplayItem::Create(picture)); + display_list->AppendItem(DrawingDisplayItem::Create(picture)); if (!it->transform.IsIdentity()) { - list->AppendItem(EndTransformDisplayItem::Create()); + display_list->AppendItem(EndTransformDisplayItem::Create()); } } @@ -118,13 +118,12 @@ FakeContentLayerClient::PaintContentsToDisplayList( recorder.beginRecording(gfx::RectFToSkRect(draw_rect))); canvas->drawRect(gfx::RectFToSkRect(draw_rect), paint); picture = skia::AdoptRef(recorder.endRecordingAsPicture()); - list->AppendItem(DrawingDisplayItem::Create(picture)); + display_list->AppendItem(DrawingDisplayItem::Create(picture)); draw_rect.Inset(1, 1); } } - list->AppendItem(EndClipDisplayItem::Create()); - return list; + display_list->AppendItem(EndClipDisplayItem::Create()); } 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 a4ecc59..d01bbd1 100644 --- a/cc/test/fake_content_layer_client.h +++ b/cc/test/fake_content_layer_client.h @@ -40,7 +40,8 @@ class FakeContentLayerClient : public ContentLayerClient { void PaintContents(SkCanvas* canvas, const gfx::Rect& rect, PaintingControlSetting painting_control) override; - scoped_refptr<DisplayItemList> PaintContentsToDisplayList( + void PaintContentsToDisplayList( + DisplayItemList* display_list, 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 679d5d6..544a022 100644 --- a/cc/test/fake_display_list_recording_source.h +++ b/cc/test/fake_display_list_recording_source.h @@ -40,9 +40,11 @@ class FakeDisplayListRecordingSource : public DisplayListRecordingSource { void Rerecord() { ContentLayerClient::PaintingControlSetting painting_control = ContentLayerClient::PAINTING_BEHAVIOR_NORMAL; - display_list_ = client_.PaintContentsToDisplayList(recorded_viewport_, - painting_control); - display_list_->set_layer_rect(recorded_viewport_); + 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_->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 f008d9f..3f4b40b 100644 --- a/cc/test/fake_video_frame_provider.cc +++ b/cc/test/fake_video_frame_provider.cc @@ -14,6 +14,11 @@ 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 50b6b27..53903c9 100644 --- a/cc/test/fake_video_frame_provider.h +++ b/cc/test/fake_video_frame_provider.h @@ -17,8 +17,10 @@ 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(const scoped_refptr<media::VideoFrame>&) override {} + void PutCurrentFrame() 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 7e97bb8..fe13526 100644 --- a/cc/test/solid_color_content_layer_client.cc +++ b/cc/test/solid_color_content_layer_client.cc @@ -25,12 +25,11 @@ void SolidColorContentLayerClient::PaintContents( paint); } -scoped_refptr<DisplayItemList> -SolidColorContentLayerClient::PaintContentsToDisplayList( +void SolidColorContentLayerClient::PaintContentsToDisplayList( + DisplayItemList* display_list, 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 1ab4c4d..3068f9ae 100644 --- a/cc/test/solid_color_content_layer_client.h +++ b/cc/test/solid_color_content_layer_client.h @@ -19,7 +19,8 @@ class SolidColorContentLayerClient : public ContentLayerClient { void PaintContents(SkCanvas* canvas, const gfx::Rect& rect, PaintingControlSetting painting_control) override; - scoped_refptr<DisplayItemList> PaintContentsToDisplayList( + void PaintContentsToDisplayList( + DisplayItemList* display_list, 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 439d920..0046b1a 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 {} - scoped_refptr<DisplayItemList> PaintContentsToDisplayList( + void PaintContentsToDisplayList( + DisplayItemList* display_list, 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 97765bd..0cb6aea 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 { } } - scoped_refptr<DisplayItemList> PaintContentsToDisplayList( + void PaintContentsToDisplayList( + DisplayItemList* display_list, const gfx::Rect& clip, PaintingControlSetting picture_control) override { NOTIMPLEMENTED(); - return DisplayItemList::Create(); } private: @@ -303,11 +303,11 @@ class CheckerContentLayerClient : public ContentLayerClient { } } } - scoped_refptr<DisplayItemList> PaintContentsToDisplayList( + void PaintContentsToDisplayList( + DisplayItemList* display_list, 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); } - scoped_refptr<DisplayItemList> PaintContentsToDisplayList( + void PaintContentsToDisplayList( + DisplayItemList* display_list, 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 db7bb9c..ff8c5e8 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); } - scoped_refptr<DisplayItemList> PaintContentsToDisplayList( + void PaintContentsToDisplayList( + DisplayItemList* display_list, 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)); } - scoped_refptr<DisplayItemList> PaintContentsToDisplayList( + void PaintContentsToDisplayList( + DisplayItemList* display_list, 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 d2a6d15..b25eca1 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,11 +71,8 @@ public class FakeServerHelper { * Deletes the existing FakeServer. */ public static void deleteFakeServer() { - if (sNativeFakeServer == 0L) { - throw new IllegalStateException( - "useFakeServer must be called before calling deleteFakeServer."); - } - + checkFakeServerInitialized( + "useFakeServer must be called before calling deleteFakeServer."); ThreadUtils.runOnUiThreadBlockingNoException(new Callable<Void>() { @Override public Void call() { @@ -127,10 +124,8 @@ public class FakeServerHelper { * @return whether the number of specified entities exist */ public boolean verifyEntityCountByTypeAndName(int count, ModelType modelType, String name) { - if (sNativeFakeServer == 0L) { - throw new IllegalStateException( + checkFakeServerInitialized( "useFakeServer must be called before data verification."); - } return nativeVerifyEntityCountByTypeAndName(mNativeFakeServerHelperAndroid, sNativeFakeServer, count, modelType.toString(), name); } @@ -144,16 +139,44 @@ public class FakeServerHelper { * @param entitySpecifics the EntitySpecifics proto that represents the entity to inject */ public void injectUniqueClientEntity(String name, EntitySpecifics entitySpecifics) { - if (sNativeFakeServer == 0L) { - throw new IllegalStateException( - "useFakeServer must be called before data injection."); - } + checkFakeServerInitialized("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); @@ -167,4 +190,9 @@ 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 730500c..27226bd 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,6 +291,23 @@ 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 afe1bf4..f949875 100644 --- a/chrome/browser/chromeos/login/helper.cc +++ b/chrome/browser/chromeos/login/helper.cc @@ -153,8 +153,20 @@ content::StoragePartition* GetSigninPartition() { } net::URLRequestContextGetter* GetSigninContext() { - if (StartupUtils::IsWebviewSigninEnabled()) - return GetSigninPartition()->GetURLRequestContext(); + 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(); + } 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 ebb23d2..d4e31b2 100644 --- a/chrome/browser/chromeos/login/hid_detection_browsertest.cc +++ b/chrome/browser/chromeos/login/hid_detection_browsertest.cc @@ -43,11 +43,15 @@ void SetUpBluetoothMock( namespace chromeos { -class HidDetectionTest : public OobeBaseTest { +// 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> { public: typedef device::InputServiceLinux::InputDeviceInfo InputDeviceInfo; HidDetectionTest() : weak_ptr_factory_(this) { + set_use_webview(GetParam()); InputServiceProxy::SetThreadIdForTesting(content::BrowserThread::UI); HidDetectionTest::InitInputService(); } @@ -112,13 +116,17 @@ class HidDetectionSkipTest : public HidDetectionTest { } }; -IN_PROC_BROWSER_TEST_F(HidDetectionTest, NoDevicesConnected) { +IN_PROC_BROWSER_TEST_P(HidDetectionTest, NoDevicesConnected) { OobeScreenWaiter(OobeDisplay::SCREEN_OOBE_HID_DETECTION).Wait(); } -IN_PROC_BROWSER_TEST_F(HidDetectionSkipTest, BothDevicesPreConnected) { +IN_PROC_BROWSER_TEST_P(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 85880b1..5d1cffa 100644 --- a/chrome/browser/chromeos/login/login_browsertest.cc +++ b/chrome/browser/chromeos/login/login_browsertest.cc @@ -5,6 +5,8 @@ #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" @@ -17,7 +19,9 @@ #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" @@ -143,6 +147,33 @@ 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() { @@ -215,14 +246,14 @@ IN_PROC_BROWSER_TEST_F(LoginSigninTest, WebUIVisible) { ASSERT_TRUE(tracing::EndTracing(&json_events)); } -IN_PROC_BROWSER_TEST_F(LoginTest, PRE_GaiaAuthOffline) { +IN_PROC_BROWSER_TEST_F(LoginOfflineTest, PRE_GaiaAuthOffline) { RegisterUser(kTestUser); chromeos::StartupUtils::MarkOobeCompleted(); chromeos::CrosSettings::Get()->SetBoolean( chromeos::kAccountsPrefShowUserNamesOnSignIn, false); } -IN_PROC_BROWSER_TEST_F(LoginTest, GaiaAuthOffline) { +IN_PROC_BROWSER_TEST_F(LoginOfflineTest, 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 45a6101..bd44832 100644 --- a/chrome/browser/chromeos/login/login_manager_test.cc +++ b/chrome/browser/chromeos/login/login_manager_test.cc @@ -5,14 +5,20 @@ #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" @@ -27,7 +33,8 @@ namespace chromeos { LoginManagerTest::LoginManagerTest(bool should_launch_browser) - : should_launch_browser_(should_launch_browser), + : use_webview_(false), + should_launch_browser_(should_launch_browser), web_contents_(NULL) { set_exit_when_last_browser_closes(false); } @@ -61,6 +68,29 @@ 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 e2b6921..ff4a6dd 100644 --- a/chrome/browser/chromeos/login/login_manager_test.h +++ b/chrome/browser/chromeos/login/login_manager_test.h @@ -33,6 +33,7 @@ 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. @@ -66,6 +67,12 @@ 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 da7e596..2f13bd1 100644 --- a/chrome/browser/chromeos/login/oobe_browsertest.cc +++ b/chrome/browser/chromeos/login/oobe_browsertest.cc @@ -5,6 +5,7 @@ #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" @@ -14,8 +15,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" @@ -27,28 +28,17 @@ using namespace net::test_server; namespace chromeos { -class OobeTest : public InProcessBrowserTest { +// 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> { public: - OobeTest() {} + OobeTest() { set_use_webview(GetParam()); } ~OobeTest() override {} void SetUpCommandLine(base::CommandLine* command_line) override { - 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(); + command_line->AppendSwitch(switches::kOobeSkipPostLogin); - base::CommandLine::ForCurrentProcess()->AppendSwitchASCII( - ::switches::kGaiaUrl, embedded_test_server()->base_url().spec()); + OobeBaseTest::SetUpCommandLine(command_line); } void TearDownOnMainThread() override { @@ -58,6 +48,8 @@ class OobeTest : public InProcessBrowserTest { base::Bind(&chrome::AttemptExit)); content::RunMessageLoop(); } + + OobeBaseTest::TearDownOnMainThread(); } chromeos::WebUILoginDisplay* GetLoginDisplay() { @@ -75,40 +67,24 @@ class OobeTest : public InProcessBrowserTest { } private: - FakeGaia fake_gaia_; DISALLOW_COPY_AND_ASSIGN(OobeTest); }; -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()); +IN_PROC_BROWSER_TEST_P(OobeTest, NewUser) { + WaitForGaiaPageLoad(); - content::WindowedNotificationObserver( - chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE, - content::NotificationService::AllSources()).Wait(); + content::WindowedNotificationObserver session_start_waiter( + chrome::NOTIFICATION_SESSION_STARTED, + content::NotificationService::AllSources()); - // TODO(glotov): mock GAIA server (test_server()) should support - // username/password configuration. - GetLoginDisplay()->ShowSigninScreenForCreds("username", "password"); + GetLoginDisplay()->ShowSigninScreenForCreds(OobeBaseTest::kFakeUserEmail, + OobeBaseTest::kFakeUserPassword); - content::WindowedNotificationObserver( - chrome::NOTIFICATION_SESSION_STARTED, - content::NotificationService::AllSources()).Wait(); + session_start_waiter.Wait(); } -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(); +IN_PROC_BROWSER_TEST_P(OobeTest, Accelerator) { + WaitForGaiaPageLoad(); gfx::NativeWindow login_window = GetLoginWindowWidget()->GetNativeWindow(); @@ -121,4 +97,6 @@ IN_PROC_BROWSER_TEST_F(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 eca6b5c..3eef72f 100644 --- a/chrome/browser/chromeos/login/proxy_auth_dialog_browsertest.cc +++ b/chrome/browser/chromeos/login/proxy_auth_dialog_browsertest.cc @@ -57,13 +57,21 @@ class ProxyAuthDialogWaiter : public content::WindowedNotificationObserver { } // namespace -class ProxyAuthOnUserBoardScreenTest : public LoginManagerTest { +// 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> { public: ProxyAuthOnUserBoardScreenTest() : LoginManagerTest(true /* should_launch_browser */), proxy_server_(net::SpawnedTestServer::TYPE_BASIC_AUTH_PROXY, net::SpawnedTestServer::kLocalhost, - base::FilePath()) {} + 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()); + } ~ProxyAuthOnUserBoardScreenTest() override {} @@ -84,13 +92,13 @@ class ProxyAuthOnUserBoardScreenTest : public LoginManagerTest { DISALLOW_COPY_AND_ASSIGN(ProxyAuthOnUserBoardScreenTest); }; -IN_PROC_BROWSER_TEST_F(ProxyAuthOnUserBoardScreenTest, +IN_PROC_BROWSER_TEST_P(ProxyAuthOnUserBoardScreenTest, PRE_ProxyAuthDialogOnUserBoardScreen) { RegisterUser("test-user@gmail.com"); StartupUtils::MarkOobeCompleted(); } -IN_PROC_BROWSER_TEST_F(ProxyAuthOnUserBoardScreenTest, +IN_PROC_BROWSER_TEST_P(ProxyAuthOnUserBoardScreenTest, ProxyAuthDialogOnUserBoardScreen) { LoginDisplayHost* login_display_host = LoginDisplayHostImpl::default_host(); WebUILoginView* web_ui_login_view = login_display_host->GetWebUILoginView(); @@ -120,4 +128,8 @@ IN_PROC_BROWSER_TEST_F(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 0a11e4c..ce4d3c0 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,9 +259,14 @@ 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) { use_webview_ = GetParam(); } + SamlTest() : saml_load_injected_(false) { + set_use_webview(GetParam()); + set_initialize_fake_merge_session(false); + } ~SamlTest() override {} void SetUpCommandLine(base::CommandLine* command_line) override { @@ -365,13 +370,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();")); @@ -397,7 +402,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 31395c2..50f75fb 100644 --- a/chrome/browser/chromeos/login/session/user_session_manager.cc +++ b/chrome/browser/chromeos/login/session/user_session_manager.cc @@ -997,14 +997,29 @@ 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_; - 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)); + + 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)); + } return; } @@ -1231,9 +1246,18 @@ void UserSessionManager::RestoreAuthSessionImpl( OAuth2LoginManagerFactory::GetInstance()->GetForProfile(profile); login_manager->AddObserver(this); - login_manager->RestoreSession( - GetAuthRequestContext(), session_restore_strategy_, - user_context_.GetRefreshToken(), user_context_.GetAuthCode()); + 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()); } void UserSessionManager::InitRlzImpl(Profile* profile, bool disabled) { @@ -1422,13 +1446,15 @@ void UserSessionManager::UpdateEasyUnlockKeys(const UserContext& user_context) { net::URLRequestContextGetter* UserSessionManager::GetAuthRequestContext() const { - net::URLRequestContextGetter* auth_request_context = NULL; + net::URLRequestContextGetter* auth_request_context = nullptr; 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. - auth_request_context = login::GetSigninPartition()->GetURLRequestContext(); + content::StoragePartition* signin_partition = login::GetSigninPartition(); + if (signin_partition) + auth_request_context = signin_partition->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 48bdb7b..c09e3a7 100644 --- a/chrome/browser/chromeos/login/signin/oauth2_browsertest.cc +++ b/chrome/browser/chromeos/login/signin/oauth2_browsertest.cc @@ -13,6 +13,7 @@ #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" @@ -130,9 +131,12 @@ class OAuth2LoginManagerStateWaiter : public OAuth2LoginManager::Observer { } // namespace -class OAuth2Test : public OobeBaseTest { +// 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> { protected: - OAuth2Test() {} + OAuth2Test() { set_use_webview(GetParam()); } void SetUpCommandLine(base::CommandLine* command_line) override { OobeBaseTest::SetUpCommandLine(command_line); @@ -327,23 +331,17 @@ class OAuth2Test : public OobeBaseTest { void StartNewUserSession(bool wait_for_merge) { SetupGaiaServerForNewAccount(); SimulateNetworkOnline(); - chromeos::WizardController::SkipPostLoginScreensForTesting(); - chromeos::WizardController* wizard_controller = - chromeos::WizardController::default_controller(); - wizard_controller->SkipToLoginForTesting(LoginScreenContext()); + WaitForGaiaPageLoad(); - content::WindowedNotificationObserver( - chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE, - content::NotificationService::AllSources()).Wait(); + content::WindowedNotificationObserver session_start_waiter( + chrome::NOTIFICATION_SESSION_STARTED, + content::NotificationService::AllSources()); // Use capitalized and dotted user name on purpose to make sure // our email normalization kicks in. GetLoginDisplay()->ShowSigninScreenForCreds(kTestRawAccountId, kTestAccountPassword); - - content::WindowedNotificationObserver( - chrome::NOTIFICATION_SESSION_STARTED, - content::NotificationService::AllSources()).Wait(); + session_start_waiter.Wait(); if (wait_for_merge) { // Wait for the session merge to finish. @@ -414,7 +412,7 @@ class CookieReader : public base::RefCountedThreadSafe<CookieReader> { }; // PRE_MergeSession is testing merge session for a new profile. -IN_PROC_BROWSER_TEST_F(OAuth2Test, PRE_PRE_PRE_MergeSession) { +IN_PROC_BROWSER_TEST_P(OAuth2Test, PRE_PRE_PRE_MergeSession) { StartNewUserSession(true); // Check for existance of refresh token. ProfileOAuth2TokenService* token_service = @@ -424,7 +422,6 @@ IN_PROC_BROWSER_TEST_F(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); @@ -435,7 +432,7 @@ IN_PROC_BROWSER_TEST_F(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_F(OAuth2Test, PRE_PRE_MergeSession) { +IN_PROC_BROWSER_TEST_P(OAuth2Test, PRE_PRE_MergeSession) { SetupGaiaServerForUnexpiredAccount(); SimulateNetworkOnline(); LoginAsExistingUser(); @@ -449,7 +446,7 @@ IN_PROC_BROWSER_TEST_F(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_F(OAuth2Test, PRE_MergeSession) { +IN_PROC_BROWSER_TEST_P(OAuth2Test, PRE_MergeSession) { SetupGaiaServerForExpiredAccount(); SimulateNetworkOnline(); LoginAsExistingUser(); @@ -465,7 +462,7 @@ IN_PROC_BROWSER_TEST_F(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_F(OAuth2Test, MergeSession) { +IN_PROC_BROWSER_TEST_P(OAuth2Test, MergeSession) { SimulateNetworkOnline(); content::WindowedNotificationObserver( @@ -535,9 +532,7 @@ 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() { @@ -699,7 +694,7 @@ Browser* FindOrCreateVisibleBrowser(Profile* profile) { return browser; } -IN_PROC_BROWSER_TEST_F(MergeSessionTest, PageThrottle) { +IN_PROC_BROWSER_TEST_P(MergeSessionTest, PageThrottle) { StartNewUserSession(false); // Try to open a page from google.com. @@ -742,7 +737,7 @@ IN_PROC_BROWSER_TEST_F(MergeSessionTest, PageThrottle) { DVLOG(1) << "Loaded page at the end : " << title; } -IN_PROC_BROWSER_TEST_F(MergeSessionTest, XHRThrottle) { +IN_PROC_BROWSER_TEST_P(MergeSessionTest, XHRThrottle) { StartNewUserSession(false); // Wait until we get send merge session request. @@ -796,4 +791,7 @@ IN_PROC_BROWSER_TEST_F(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 1a0d47f..c136ea3 100644 --- a/chrome/browser/chromeos/login/startup_utils.cc +++ b/chrome/browser/chromeos/login/startup_utils.cc @@ -13,6 +13,7 @@ #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" @@ -59,7 +60,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::kWebviewSigninEnabled, false); + registry->RegisterBooleanPref(prefs::kWebviewSigninDisabled, false); } // static @@ -179,25 +180,41 @@ std::string StartupUtils::GetInitialLocale() { // static bool StartupUtils::IsWebviewSigninAllowed() { - return extensions::GetCurrentChannel() <= chrome::VersionInfo::CHANNEL_DEV && - !base::CommandLine::ForCurrentProcess()->HasSwitch( - switches::kDisableWebviewSigninFlow); + return !base::CommandLine::ForCurrentProcess()->HasSwitch( + switches::kDisableWebviewSigninFlow); } // static bool StartupUtils::IsWebviewSigninEnabled() { - return IsWebviewSigninAllowed() && - g_browser_process->local_state()->GetBoolean( - prefs::kWebviewSigninEnabled); + 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; } // static bool StartupUtils::EnableWebviewSignin(bool is_enabled) { - if (!IsWebviewSigninAllowed()) + if (is_enabled && !IsWebviewSigninAllowed()) return false; - g_browser_process->local_state()->SetBoolean(prefs::kWebviewSigninEnabled, - is_enabled); + g_browser_process->local_state()->SetBoolean(prefs::kWebviewSigninDisabled, + !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 5dcc635..5aa0eff 100644 --- a/chrome/browser/chromeos/login/test/oobe_base_test.cc +++ b/chrome/browser/chromeos/login/test/oobe_base_test.cc @@ -20,6 +20,7 @@ #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" @@ -32,12 +33,19 @@ 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) { + use_webview_(false), + initialize_fake_merge_session_(true) { set_exit_when_last_browser_closes(false); set_chromeos_user_ = false; } @@ -50,6 +58,8 @@ 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()))); @@ -67,25 +77,19 @@ void OobeBaseTest::SetUp() { } bool OobeBaseTest::SetUpUserDataDirectory() { - if (use_webview_) { - // Fake Dev channel to enable webview signin. - scoped_channel_.reset( - new extensions::ScopedCurrentChannel(chrome::VersionInfo::CHANNEL_DEV)); + 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::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. + 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; - 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); + + // 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)); @@ -106,6 +110,11 @@ 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(); @@ -123,6 +132,7 @@ void OobeBaseTest::TearDownOnMainThread() { base::Bind(&chrome::AttemptExit)); content::RunMessageLoop(); } + EXPECT_TRUE(embedded_test_server()->ShutdownAndWaitUntilComplete()); ExtensionApiTest::TearDownOnMainThread(); } @@ -136,14 +146,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(""); + GURL gaia_url = gaia_https_forwarder_->GetURL(std::string()); 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() { @@ -152,6 +162,9 @@ 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; @@ -217,11 +230,32 @@ 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 6933a01..7c2f592 100644 --- a/chrome/browser/chromeos/login/test/oobe_base_test.h +++ b/chrome/browser/chromeos/login/test/oobe_base_test.h @@ -34,9 +34,22 @@ 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; @@ -60,12 +73,23 @@ 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, @@ -83,6 +107,7 @@ 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 c3163b2..b7d3caa 100644 --- a/chrome/browser/chromeos/login/webview_login_browsertest.cc +++ b/chrome/browser/chromeos/login/webview_login_browsertest.cc @@ -10,57 +10,27 @@ #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() { use_webview_ = true; } + WebviewLoginTest() { set_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) { - WaitForGaiaPageLoaded(); + WaitForGaiaPageLoad(); JsExpect("$('close-button-item').hidden"); - SetSignFormField("identifier", kFakeUserEmail); + SetSignFormField("identifier", OobeBaseTest::kFakeUserEmail); ExecuteJsInSigninFrame("document.getElementById('nextButton').click();"); JsExpect("$('close-button-item').hidden"); @@ -69,21 +39,21 @@ IN_PROC_BROWSER_TEST_F(WebviewLoginTest, Basic) { chrome::NOTIFICATION_SESSION_STARTED, content::NotificationService::AllSources()); - SetSignFormField("password", kFakeUserPassword); + SetSignFormField("password", OobeBaseTest::kFakeUserPassword); ExecuteJsInSigninFrame("document.getElementById('nextButton').click();"); session_start_waiter.Wait(); } IN_PROC_BROWSER_TEST_F(WebviewLoginTest, BackButton) { - WaitForGaiaPageLoaded(); + WaitForGaiaPageLoad(); // 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", kFakeUserEmail); + SetSignFormField("identifier", OobeBaseTest::kFakeUserEmail); ExecuteJsInSigninFrame("document.getElementById('nextButton').click();"); JsExpect("!$('back-button-item').hidden"); JsExpect("$('signin-frame').src.indexOf('#challengepassword') != -1"); @@ -103,7 +73,7 @@ IN_PROC_BROWSER_TEST_F(WebviewLoginTest, BackButton) { chrome::NOTIFICATION_SESSION_STARTED, content::NotificationService::AllSources()); - SetSignFormField("password", kFakeUserPassword); + SetSignFormField("password", OobeBaseTest::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 658a78a..3b71746 100644 --- a/chrome/browser/chromeos/login/wizard_controller_browsertest.cc +++ b/chrome/browser/chromeos/login/wizard_controller_browsertest.cc @@ -7,6 +7,8 @@ #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" @@ -48,6 +50,7 @@ #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" @@ -965,7 +968,11 @@ IN_PROC_BROWSER_TEST_F(WizardControllerBrokenLocalStateTest, ASSERT_EQ(1, fake_session_manager_client()->start_device_wipe_call_count()); } -class WizardControllerProxyAuthOnSigninTest : public WizardControllerTest { +// 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> { protected: WizardControllerProxyAuthOnSigninTest() : proxy_server_(net::SpawnedTestServer::TYPE_BASIC_AUTH_PROXY, @@ -991,6 +998,32 @@ class WizardControllerProxyAuthOnSigninTest : public WizardControllerTest { 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: @@ -999,7 +1032,7 @@ class WizardControllerProxyAuthOnSigninTest : public WizardControllerTest { DISALLOW_COPY_AND_ASSIGN(WizardControllerProxyAuthOnSigninTest); }; -IN_PROC_BROWSER_TEST_F(WizardControllerProxyAuthOnSigninTest, +IN_PROC_BROWSER_TEST_P(WizardControllerProxyAuthOnSigninTest, ProxyAuthDialogOnSigninScreen) { content::WindowedNotificationObserver auth_needed_waiter( chrome::NOTIFICATION_AUTH_NEEDED, @@ -1011,6 +1044,10 @@ IN_PROC_BROWSER_TEST_F(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 dabc1f7..03b0504 100644 --- a/chrome/browser/chromeos/policy/blocking_login_browsertest.cc +++ b/chrome/browser/chromeos/policy/blocking_login_browsertest.cc @@ -12,6 +12,7 @@ #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" @@ -27,7 +28,6 @@ #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,52 +81,35 @@ struct BlockingLoginTestParam { }; class BlockingLoginTest - : public InProcessBrowserTest, + : public OobeBaseTest, public content::NotificationObserver, public testing::WithParamInterface<BlockingLoginTestParam> { public: BlockingLoginTest() : profile_added_(NULL) {} void SetUpCommandLine(base::CommandLine* command_line) override { - // 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"); + OobeBaseTest::SetUpCommandLine(command_line); + 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_); - EXPECT_TRUE(embedded_test_server()->ShutdownAndWaitUntilComplete()); + OobeBaseTest::TearDownOnMainThread(); } void Observe(int type, @@ -245,10 +228,14 @@ 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 47dd710..f6967bd 100644 --- a/chrome/browser/resources/extensions/extension_list.js +++ b/chrome/browser/resources/extensions/extension_list.js @@ -285,32 +285,11 @@ 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; @@ -360,6 +339,14 @@ 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 3eba3da..211ecba 100644 --- a/chrome/browser/resources/extensions/extensions.js +++ b/chrome/browser/resources/extensions/extensions.js @@ -263,6 +263,7 @@ 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.html b/chrome/browser/resources/settings/a11y_page/a11y_page.html index cf7a15a..8076bbd 100644 --- a/chrome/browser/resources/settings/a11y_page/a11y_page.html +++ b/chrome/browser/resources/settings/a11y_page/a11y_page.html @@ -5,11 +5,12 @@ <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"> - <link rel="stylesheet" href="a11y_page.css"> + <core-style ref="a11yPageStyle"></core-style> <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/a11y_page/a11y_page.css b/chrome/browser/resources/settings/a11y_page/a11y_page_style.html index 2277ed3..ff6c3bd 100644 --- a/chrome/browser/resources/settings/a11y_page/a11y_page.css +++ b/chrome/browser/resources/settings/a11y_page/a11y_page_style.html @@ -1,7 +1,7 @@ -/* 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. */ +<link rel="import" href="chrome://resources/polymer/polymer/polymer.html"> +<link rel="import" href="chrome://resources/polymer/core-style/core-style.html"> +<core-style id="a11yPageStyle"> .autoclick-delay-label { -webkit-margin-end: 0; -webkit-margin-start: 40px; @@ -21,3 +21,4 @@ .more-a11y-link { margin-bottom: 10px; } +</core-style> diff --git a/chrome/browser/resources/settings/checkbox/checkbox.html b/chrome/browser/resources/settings/checkbox/checkbox.html index e660fdb..e4e884c 100644 --- a/chrome/browser/resources/settings/checkbox/checkbox.html +++ b/chrome/browser/resources/settings/checkbox/checkbox.html @@ -2,10 +2,11 @@ <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> - <link rel="stylesheet" href="checkbox.css"> + <core-style ref="checkboxStyle"></core-style> <cr-events id="events"></cr-events> <cr-settings-pref-tracker pref="{{pref}}"></cr-settings-pref-tracker> diff --git a/chrome/browser/resources/settings/checkbox/checkbox.css b/chrome/browser/resources/settings/checkbox/checkbox_style.html index 0acbdbd..550eda4 100644 --- a/chrome/browser/resources/settings/checkbox/checkbox.css +++ b/chrome/browser/resources/settings/checkbox/checkbox_style.html @@ -1,7 +1,7 @@ -/* 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. */ +<link rel="import" href="chrome://resources/polymer/polymer/polymer.html"> +<link rel="import" href="chrome://resources/polymer/core-style/core-style.html"> +<core-style id="checkboxStyle"> #checkbox { -webkit-margin-end: 10px; } @@ -17,3 +17,4 @@ core-label { -webkit-margin-start: 10px; color: rgba(0, 0, 0, .5); } +</core-style> diff --git a/chrome/browser/resources/settings/settings_resources.grd b/chrome/browser/resources/settings/settings_resources.grd index 1012730..5ceed83 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_CSS" - file="a11y_page/a11y_page.css" + <structure name="IDR_SETTINGS_A11Y_PAGE_STYLE_HTML" + file="a11y_page/a11y_page_style.html" 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_CSS" - file="checkbox/checkbox.css" + <structure name="IDR_SETTINGS_CHECKBOX_STYLE_HTML" + file="checkbox/checkbox_style.html" 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 0657d6f..9d65d26 100644 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -1081,6 +1081,10 @@ '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 2966dbf..c3678fc 100644 --- a/chrome/common/pref_names.cc +++ b/chrome/common/pref_names.cc @@ -1919,9 +1919,8 @@ const char kConsumerManagementStage[] = "consumer_management.stage"; const char kNewOobe[] = "NewOobe"; // A boolean pref. If set to true, experimental webview based signin flow -// activated. -const char kWebviewSigninEnabled[] = - "webview_signin_enabled"; +// is deactivated. +const char kWebviewSigninDisabled[] = "webview_signin_disabled"; #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 b40b4f0..3badee1 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 kWebviewSigninEnabled[]; +extern const char kWebviewSigninDisabled[]; #endif // defined(OS_CHROMEOS) extern const char kClearPluginLSODataEnabled[]; diff --git a/chrome/interactive_ui_tests.isolate b/chrome/interactive_ui_tests.isolate index 01c2675..9823d0c 100644 --- a/chrome/interactive_ui_tests.isolate +++ b/chrome/interactive_ui_tests.isolate @@ -130,6 +130,7 @@ '<(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 7c4eb86..d0b4124 100644 --- a/chrome/test/data/notifications/platform_notification_service.html +++ b/chrome/test/data/notifications/platform_notification_service.html @@ -54,7 +54,10 @@ body: 'Contents', tag: 'replace-id', icon: 'icon.png', - silent: true + silent: true, + data: [ + { property: 'value' } + ] }); } diff --git a/chromecast/media/base/switching_media_renderer.cc b/chromecast/media/base/switching_media_renderer.cc index 1ca45b2..79b7d1ef 100644 --- a/chromecast/media/base/switching_media_renderer.cc +++ b/chromecast/media/base/switching_media_renderer.cc @@ -29,7 +29,6 @@ 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) { @@ -53,7 +52,7 @@ void SwitchingMediaRenderer::Initialize( return GetRenderer()->Initialize( demuxer_stream_provider, init_cb, statistics_cb, buffering_state_cb, - paint_cb, ended_cb, error_cb, waiting_for_decryption_key_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 830cef6..2739f40 100644 --- a/chromecast/media/base/switching_media_renderer.h +++ b/chromecast/media/base/switching_media_renderer.h @@ -36,7 +36,6 @@ 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 1473f02..bc29b4f 100644 --- a/chromecast/media/cma/filters/cma_renderer.cc +++ b/chromecast/media/cma/filters/cma_renderer.cc @@ -22,6 +22,7 @@ #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 { @@ -36,12 +37,14 @@ const base::TimeDelta kMaxDeltaFetcher( } // namespace -CmaRenderer::CmaRenderer(scoped_ptr<MediaPipeline> media_pipeline) +CmaRenderer::CmaRenderer(scoped_ptr<MediaPipeline> media_pipeline, + ::media::VideoRendererSink* video_renderer_sink) : 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), @@ -76,7 +79,6 @@ 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) { @@ -97,7 +99,6 @@ 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. @@ -160,7 +161,8 @@ void CmaRenderer::StartPlayingFrom(base::TimeDelta time) { // Pipeline::StateTransitionTask). if (!initial_video_hole_created_) { initial_video_hole_created_ = true; - paint_cb_.Run(::media::VideoFrame::CreateHoleFrame(initial_natural_size_)); + video_renderer_sink_->PaintFrameUsingOldRenderingPath( + ::media::VideoFrame::CreateHoleFrame(initial_natural_size_)); } #endif @@ -383,7 +385,8 @@ void CmaRenderer::OnStatisticsUpdated( void CmaRenderer::OnNaturalSizeChanged(const gfx::Size& size) { DCHECK(thread_checker_.CalledOnValidThread()); #if defined(VIDEO_HOLE) - paint_cb_.Run(::media::VideoFrame::CreateHoleFrame(size)); + video_renderer_sink_->PaintFrameUsingOldRenderingPath( + ::media::VideoFrame::CreateHoleFrame(size)); #endif } diff --git a/chromecast/media/cma/filters/cma_renderer.h b/chromecast/media/cma/filters/cma_renderer.h index 62efd7c..25fec41 100644 --- a/chromecast/media/cma/filters/cma_renderer.h +++ b/chromecast/media/cma/filters/cma_renderer.h @@ -23,6 +23,7 @@ namespace media { class DemuxerStreamProvider; class TimeDeltaInterpolator; class VideoFrame; +class VideoRendererSink; } namespace chromecast { @@ -34,7 +35,8 @@ class VideoPipeline; class CmaRenderer : public ::media::Renderer { public: - explicit CmaRenderer(scoped_ptr<MediaPipeline> media_pipeline); + CmaRenderer(scoped_ptr<MediaPipeline> media_pipeline, + ::media::VideoRendererSink* video_renderer_sink); ~CmaRenderer() override; // ::media::Renderer implementation: @@ -43,7 +45,6 @@ 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; @@ -98,11 +99,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 cdfab39..62fe771 100644 --- a/chromecast/renderer/media/chromecast_media_renderer_factory.cc +++ b/chromecast/renderer/media/chromecast_media_renderer_factory.cc @@ -29,7 +29,8 @@ ChromecastMediaRendererFactory::~ChromecastMediaRendererFactory() { scoped_ptr<::media::Renderer> ChromecastMediaRendererFactory::CreateRenderer( const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner, - ::media::AudioRendererSink* audio_renderer_sink) { + ::media::AudioRendererSink* audio_renderer_sink, + ::media::VideoRendererSink* video_renderer_sink) { if (!default_render_factory_) { // Chromecast doesn't have input audio devices, so leave this uninitialized ::media::AudioParameters input_audio_params; @@ -61,10 +62,11 @@ scoped_ptr<::media::Renderer> ChromecastMediaRendererFactory::CreateRenderer( content::RenderThread::Get()->GetIOMessageLoopProxy(), cma_load_type)); scoped_ptr<CmaRenderer> cma_renderer( - new CmaRenderer(cma_media_pipeline.Pass())); + new CmaRenderer(cma_media_pipeline.Pass(), video_renderer_sink)); scoped_ptr<::media::Renderer> default_media_render( default_render_factory_->CreateRenderer(media_task_runner, - audio_renderer_sink)); + audio_renderer_sink, + video_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 f880ff6..8206094 100644 --- a/chromecast/renderer/media/chromecast_media_renderer_factory.h +++ b/chromecast/renderer/media/chromecast_media_renderer_factory.h @@ -27,7 +27,8 @@ 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) final; + ::media::AudioRendererSink* audio_renderer_sink, + ::media::VideoRendererSink* video_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 9ee5108..e29759a 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,10 +11,14 @@ #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" @@ -46,9 +50,82 @@ 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, @@ -60,13 +137,11 @@ 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); } @@ -75,15 +150,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; @@ -256,9 +331,13 @@ void DataReductionProxyConfig::SetProxyConfig( if (enabled && !(alternative_enabled && !config_values_->alternative_fallback_allowed())) { - ui_task_runner_->PostTask( - FROM_HERE, base::Bind(&DataReductionProxyConfig::StartSecureProxyCheck, - base::Unretained(this))); + // 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))); } } @@ -322,16 +401,7 @@ void DataReductionProxyConfig::LogProxyState(bool enabled, void DataReductionProxyConfig::HandleSecureProxyCheckResponse( const std::string& response, const net::URLRequestStatus& status) { - 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) { + DCHECK(io_task_runner_->BelongsToCurrentThread()); if (event_store_) { event_store_->EndSecureProxyCheck(bound_net_log_, status.error()); } @@ -394,9 +464,13 @@ void DataReductionProxyConfig::OnIPAddressChanged() { return; } - ui_task_runner_->PostTask( - FROM_HERE, base::Bind(&DataReductionProxyConfig::StartSecureProxyCheck, - base::Unretained(this))); + // 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))); } } @@ -432,21 +506,21 @@ void DataReductionProxyConfig::RecordSecureProxyCheckFetchResult( SECURE_PROXY_CHECK_FETCH_RESULT_COUNT); } -void DataReductionProxyConfig::StartSecureProxyCheck() { - DCHECK(ui_task_runner_->BelongsToCurrentThread()); +void DataReductionProxyConfig::SecureProxyCheck( + const GURL& secure_proxy_check_url, + FetcherResponseCallback fetcher_callback) { + DCHECK(io_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()); - } - data_reduction_proxy_service_->SecureProxyCheck( - config_values_->secure_proxy_check_url(), - base::Bind(&DataReductionProxyConfig::HandleSecureProxyCheckResponse, - base::Unretained(this))); + if (event_store_) { + event_store_->BeginSecureProxyCheck( + bound_net_log_, config_values_->secure_proxy_check_url()); } + + 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 770c032..9acf691 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,6 +5,9 @@ #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" @@ -17,6 +20,8 @@ #include "net/proxy/proxy_config.h" #include "net/proxy/proxy_retry_info.h" +class GURL; + namespace base { class SingleThreadTaskRunner; class TimeDelta; @@ -25,6 +30,7 @@ class TimeDelta; namespace net { class HostPortPair; class NetLog; +class URLFetcher; class URLRequest; class URLRequestContextGetter; class URLRequestStatus; @@ -32,10 +38,14 @@ 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. @@ -75,16 +85,12 @@ 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); @@ -215,16 +221,16 @@ class DataReductionProxyConfig bool restricted, bool at_startup); - // Begins a secure proxy check to determine if the Data Reduction Proxy is - // permitted to use the HTTPS proxy servers. - void StartSecureProxyCheck(); + // 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); // 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(); @@ -246,6 +252,8 @@ 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_; @@ -259,10 +267,6 @@ 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 @@ -283,11 +287,6 @@ 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 f6e836c..dbbde52 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,7 +39,6 @@ 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 fba88ee..759b29c 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,6 +116,9 @@ 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 44746c1..7b61eed 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,13 +127,11 @@ 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(*service, SecureProxyCheck(_, _)) + EXPECT_CALL(*config(), SecureProxyCheck(_, _)) .Times(1) .WillRepeatedly(testing::WithArgs<1>( testing::Invoke(&responder, &TestResponder::ExecuteCallback))); @@ -161,9 +159,8 @@ class DataReductionProxyConfigTest : public testing::Test { scoped_ptr<DataReductionProxyParams> params) { params->EnableQuic(false); return make_scoped_ptr(new DataReductionProxyConfig( - test_context_->task_runner(), test_context_->task_runner(), - test_context_->net_log(), params.Pass(), test_context_->configurator(), - test_context_->event_store())); + 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 52340f7..9034bae 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_, ui_task_runner_, net_log, mutable_config.Pass(), - configurator_.get(), event_store_.get())); + io_task_runner_, net_log, mutable_config.Pass(), configurator_.get(), + event_store_.get())); } else { - config_.reset(new DataReductionProxyConfig( - io_task_runner_, ui_task_runner_, net_log, params.Pass(), - configurator_.get(), event_store_.get())); + config_.reset( + new DataReductionProxyConfig(io_task_runner_, net_log, params.Pass(), + configurator_.get(), event_store_.get())); } // It is safe to use base::Unretained here, since it gets executed @@ -113,7 +113,6 @@ 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 3c17aa2..1b90a14 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,9 +11,6 @@ #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 { @@ -109,47 +106,4 @@ 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 0f9aed7..ed39307 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,7 +14,6 @@ #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; @@ -25,16 +24,11 @@ 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; @@ -42,8 +36,7 @@ class DataReductionProxySettings; // Contains and initializes all Data Reduction Proxy objects that have a // lifetime based on the UI thread. -class DataReductionProxyService : public base::NonThreadSafe, - public net::URLFetcherDelegate { +class DataReductionProxyService : public base::NonThreadSafe { public: // The caller must ensure that |settings| and |request_context| remain alive // for the lifetime of the |DataReductionProxyService| instance. This instance @@ -56,7 +49,7 @@ class DataReductionProxyService : public base::NonThreadSafe, net::URLRequestContextGetter* request_context_getter, scoped_refptr<base::SingleThreadTaskRunner> io_task_runner); - ~DataReductionProxyService() override; + virtual ~DataReductionProxyService(); // Sets the DataReductionProxyIOData weak pointer. void SetIOData(base::WeakPtr<DataReductionProxyIOData> io_data); @@ -67,12 +60,6 @@ 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( @@ -114,22 +101,9 @@ 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 0b404ad..1d64a4c 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,6 +194,18 @@ 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 7b2add4..4993ffe 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,9 +156,6 @@ 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 8b02197..1fb6808 100644 --- a/content/browser/notifications/notification_database_data.proto +++ b/content/browser/notifications/notification_database_data.proto @@ -35,6 +35,7 @@ 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 d183dd4..d656f7d 100644 --- a/content/browser/notifications/notification_database_data_conversions.cc +++ b/content/browser/notifications/notification_database_data_conversions.cc @@ -40,6 +40,11 @@ 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; } @@ -64,6 +69,11 @@ 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 df147d0..f57bfa8 100644 --- a/content/browser/notifications/notification_database_data_unittest.cc +++ b/content/browser/notifications/notification_database_data_unittest.cc @@ -19,8 +19,12 @@ 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 = @@ -30,6 +34,7 @@ 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; @@ -64,6 +69,10 @@ 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 a1b8128..639e555 100644 --- a/content/child/notifications/notification_data_conversions.cc +++ b/content/child/notifications/notification_data_conversions.cc @@ -25,6 +25,7 @@ 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; } @@ -43,6 +44,7 @@ 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 f6a1424..daded25 100644 --- a/content/child/notifications/notification_data_conversions_unittest.cc +++ b/content/child/notifications/notification_data_conversions_unittest.cc @@ -4,6 +4,8 @@ #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" @@ -12,17 +14,18 @@ #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)); + blink::WebNotificationData web_data( blink::WebString::fromUTF8(kNotificationTitle), blink::WebNotificationData::DirectionLeftToRight, @@ -30,7 +33,8 @@ TEST(NotificationDataConversionsTest, ToPlatformNotificationData) { blink::WebString::fromUTF8(kNotificationBody), blink::WebString::fromUTF8(kNotificationTag), blink::WebURL(GURL(kNotificationIconUrl)), - true /* silent */); + true /* silent */, + blink::WebVector<char>(developer_data)); PlatformNotificationData platform_data = ToPlatformNotificationData(web_data); EXPECT_EQ(base::ASCIIToUTF16(kNotificationTitle), platform_data.title); @@ -41,6 +45,10 @@ 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, @@ -60,6 +68,9 @@ 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 = @@ -69,6 +80,7 @@ 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); @@ -79,6 +91,10 @@ 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 96963df..b442baf 100644 --- a/content/child/notifications/notification_manager.cc +++ b/content/child/notifications/notification_manager.cc @@ -4,7 +4,10 @@ #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" @@ -95,6 +98,24 @@ 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 a4ae706..7ad1e6b 100644 --- a/content/common/platform_notification_messages.h +++ b/content/common/platform_notification_messages.h @@ -43,6 +43,7 @@ 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 5b918692..387dee6 100644 --- a/content/public/common/platform_notification_data.cc +++ b/content/public/common/platform_notification_data.cc @@ -6,9 +6,7 @@ namespace content { -PlatformNotificationData::PlatformNotificationData() - : direction(NotificationDirectionLeftToRight), - silent(false) {} +PlatformNotificationData::PlatformNotificationData() {} PlatformNotificationData::~PlatformNotificationData() {} diff --git a/content/public/common/platform_notification_data.h b/content/public/common/platform_notification_data.h index f6c999d..5b608c0 100644 --- a/content/public/common/platform_notification_data.h +++ b/content/public/common/platform_notification_data.h @@ -6,6 +6,7 @@ #define CONTENT_PUBLIC_COMMON_PLATFORM_NOTIFICATION_DATA_H_ #include <string> +#include <vector> #include "base/strings/string16.h" #include "content/common/content_export.h" @@ -20,6 +21,10 @@ 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, @@ -31,7 +36,7 @@ struct CONTENT_EXPORT PlatformNotificationData { base::string16 title; // Hint to determine the directionality of the displayed notification. - NotificationDirection direction; + NotificationDirection direction = NotificationDirectionLeftToRight; // BCP 47 language tag describing the notification's contents. Optional. std::string lang; @@ -48,7 +53,11 @@ struct CONTENT_EXPORT PlatformNotificationData { // Whether default notification indicators (sound, vibration, light) should // be suppressed. - bool silent; + 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; }; } // namespace content diff --git a/content/renderer/media/android/webmediaplayer_android.cc b/content/renderer/media/android/webmediaplayer_android.cc index 7779004..284123a 100644 --- a/content/renderer/media/android/webmediaplayer_android.cc +++ b/content/renderer/media/android/webmediaplayer_android.cc @@ -1272,6 +1272,12 @@ 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; { @@ -1282,8 +1288,7 @@ scoped_refptr<media::VideoFrame> WebMediaPlayerAndroid::GetCurrentFrame() { return video_frame; } -void WebMediaPlayerAndroid::PutCurrentFrame( - const scoped_refptr<media::VideoFrame>& frame) { +void WebMediaPlayerAndroid::PutCurrentFrame() { } void WebMediaPlayerAndroid::ResetStreamTextureProxy() { diff --git a/content/renderer/media/android/webmediaplayer_android.h b/content/renderer/media/android/webmediaplayer_android.h index c1e30101..5d5f653 100644 --- a/content/renderer/media/android/webmediaplayer_android.h +++ b/content/renderer/media/android/webmediaplayer_android.h @@ -185,8 +185,10 @@ 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(const scoped_refptr<media::VideoFrame>& frame) override; + void PutCurrentFrame() 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 f76d616..de6f33b 100644 --- a/content/renderer/media/webmediaplayer_ms.cc +++ b/content/renderer/media/webmediaplayer_ms.cc @@ -456,6 +456,14 @@ 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_); @@ -467,8 +475,7 @@ scoped_refptr<media::VideoFrame> WebMediaPlayerMS::GetCurrentFrame() { return current_frame_; } -void WebMediaPlayerMS::PutCurrentFrame( - const scoped_refptr<media::VideoFrame>& frame) { +void WebMediaPlayerMS::PutCurrentFrame() { 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 da99ef1..7310fd6 100644 --- a/content/renderer/media/webmediaplayer_ms.h +++ b/content/renderer/media/webmediaplayer_ms.h @@ -134,8 +134,10 @@ 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(const scoped_refptr<media::VideoFrame>& frame) override; + void PutCurrentFrame() override; private: // The callback for VideoFrameProvider to signal a new frame is available. diff --git a/media/BUILD.gn b/media/BUILD.gn index 27ef3aa..9914df2 100644 --- a/media/BUILD.gn +++ b/media/BUILD.gn @@ -792,34 +792,3 @@ 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 9cbe50c..37940e6 100644 --- a/media/base/mock_filters.h +++ b/media/base/mock_filters.h @@ -121,17 +121,16 @@ class MockVideoRenderer : public VideoRenderer { virtual ~MockVideoRenderer(); // VideoRenderer implementation. - 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_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_METHOD1(Flush, void(const base::Closure& callback)); MOCK_METHOD1(StartPlayingFrom, void(base::TimeDelta)); MOCK_METHOD1(OnTimeStateChanged, void(bool)); @@ -170,12 +169,11 @@ class MockRenderer : public Renderer { virtual ~MockRenderer(); // Renderer implementation. - MOCK_METHOD8(Initialize, + MOCK_METHOD7(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 aaa6cb6..c39ddcd 100644 --- a/media/base/pipeline.cc +++ b/media/base/pipeline.cc @@ -68,7 +68,6 @@ 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) { @@ -77,7 +76,6 @@ 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"; @@ -90,7 +88,6 @@ 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; @@ -714,7 +711,6 @@ 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 30707b3..f10bc39 100644 --- a/media/base/pipeline.h +++ b/media/base/pipeline.h @@ -100,8 +100,6 @@ 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. @@ -115,7 +113,6 @@ 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); @@ -360,7 +357,6 @@ 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 093b51e..09d9fdd 100644 --- a/media/base/pipeline_unittest.cc +++ b/media/base/pipeline_unittest.cc @@ -84,7 +84,6 @@ 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: @@ -170,9 +169,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<5>(&ended_cb_), + SaveArg<4>(&ended_cb_), PostCallback<1>(PIPELINE_OK))); EXPECT_CALL(*renderer_, HasAudio()).WillRepeatedly(Return(audio_stream())); EXPECT_CALL(*renderer_, HasVideo()).WillRepeatedly(Return(video_stream())); @@ -196,8 +195,6 @@ 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)), @@ -858,13 +855,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)); } @@ -873,7 +870,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 e3647f7..31661c4 100644 --- a/media/base/renderer.h +++ b/media/base/renderer.h @@ -38,9 +38,6 @@ 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 @@ -50,7 +47,6 @@ 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 211447e..aea3a53 100644 --- a/media/base/renderer_factory.h +++ b/media/base/renderer_factory.h @@ -18,6 +18,7 @@ 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 { @@ -28,10 +29,12 @@ 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 the |audio_renderer_sink| to render audio. + // The created Renderer can use |audio_renderer_sink| to render audio and + // |video_renderer_sink| to render video. virtual scoped_ptr<Renderer> CreateRenderer( const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner, - AudioRendererSink* audio_renderer_sink) = 0; + AudioRendererSink* audio_renderer_sink, + VideoRendererSink* video_renderer_sink) = 0; private: DISALLOW_COPY_AND_ASSIGN(RendererFactory); diff --git a/media/base/video_renderer.h b/media/base/video_renderer.h index 46e842e..96a561f 100644 --- a/media/base/video_renderer.h +++ b/media/base/video_renderer.h @@ -45,9 +45,6 @@ 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. @@ -63,7 +60,6 @@ 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 new file mode 100644 index 0000000..a2852ff --- /dev/null +++ b/media/base/video_renderer_sink.h @@ -0,0 +1,64 @@ +// 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 d832546..e58162e 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 08a424d..fe8e04c 100644 --- a/media/base/wall_clock_time_source_unittest.cc +++ b/media/base/wall_clock_time_source_unittest.cc @@ -13,6 +13,7 @@ class WallClockTimeSourceTest : public testing::Test { WallClockTimeSourceTest() : tick_clock_(new base::SimpleTestTickClock()) { time_source_.SetTickClockForTesting( scoped_ptr<base::TickClock>(tick_clock_)); + AdvanceTimeInSeconds(1); } ~WallClockTimeSourceTest() override {} @@ -28,9 +29,18 @@ class WallClockTimeSourceTest : public testing::Test { return time_source_.SetMediaTime(base::TimeDelta::FromSeconds(seconds)); } - WallClockTimeSource time_source_; + bool IsWallClockNowForMediaTimeInSeconds(int seconds) { + return tick_clock_->NowTicks() == + time_source_.GetWallClockTime(base::TimeDelta::FromSeconds(seconds)); + } - private: + bool IsWallClockNullForMediaTimeInSeconds(int seconds) { + return time_source_.GetWallClockTime(base::TimeDelta::FromSeconds(seconds)) + .is_null(); + } + + protected: + WallClockTimeSource time_source_; base::SimpleTestTickClock* tick_clock_; // Owned by |time_source_|. DISALLOW_COPY_AND_ASSIGN(WallClockTimeSourceTest); @@ -38,26 +48,33 @@ 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) { @@ -65,26 +82,33 @@ 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 447b6b1..90af145 100644 --- a/media/blink/video_frame_compositor.cc +++ b/media/blink/video_frame_compositor.cc @@ -4,6 +4,8 @@ #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 { @@ -32,35 +34,112 @@ 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) - : natural_size_changed_cb_(natural_size_changed_cb), + : compositor_task_runner_(compositor_task_runner), + natural_size_changed_cb_(natural_size_changed_cb), opacity_changed_cb_(opacity_changed_cb), - client_(NULL) { + client_(nullptr), + rendering_(false), + callback_(nullptr) { } 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( - const scoped_refptr<VideoFrame>& frame) { +void VideoFrameCompositor::PutCurrentFrame() { + DCHECK(compositor_task_runner_->BelongsToCurrentThread()); + // TODO(dalecurtis): Wire up a flag for RenderCallback::OnFrameDropped(). } -void VideoFrameCompositor::UpdateCurrentFrame( +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::PaintFrameUsingOldRenderingPath( 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 9e46663e..02b6077 100644 --- a/media/blink/video_frame_compositor.h +++ b/media/blink/video_frame_compositor.h @@ -7,24 +7,45 @@ #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 handles incoming frames by notifying the compositor and -// dispatching callbacks when detecting changes in video frames. +// 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. // -// 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 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 // -// VideoFrameCompositor must live on the same thread as the compositor. +// 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. class MEDIA_EXPORT VideoFrameCompositor - : NON_EXPORTED_BASE(public cc::VideoFrameProvider) { + : public VideoRendererSink, + 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 @@ -34,31 +55,62 @@ class MEDIA_EXPORT VideoFrameCompositor // called the first time UpdateCurrentFrame() is called. Run on the same // thread as the caller of UpdateCurrentFrame(). // - // TODO(scherkus): Investigate the inconsistency between the callbacks with + // TODO(dalecurtis): 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; - // cc::VideoFrameProvider implementation. + // 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_|. 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(const scoped_refptr<VideoFrame>& frame) override; + void PutCurrentFrame() override; - // Updates the current frame and notifies the compositor. - void UpdateCurrentFrame(const scoped_refptr<VideoFrame>& frame); + // 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; 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_; + DISALLOW_COPY_AND_ASSIGN(VideoFrameCompositor); }; diff --git a/media/blink/video_frame_compositor_unittest.cc b/media/blink/video_frame_compositor_unittest.cc index 43d750a..b95a5000 100644 --- a/media/blink/video_frame_compositor_unittest.cc +++ b/media/blink/video_frame_compositor_unittest.cc @@ -3,6 +3,7 @@ // 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" @@ -15,6 +16,7 @@ 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, @@ -41,6 +43,8 @@ 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_; } @@ -56,6 +60,7 @@ 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_; @@ -70,12 +75,12 @@ TEST_F(VideoFrameCompositorTest, InitialValues) { EXPECT_FALSE(compositor()->GetCurrentFrame().get()); } -TEST_F(VideoFrameCompositorTest, UpdateCurrentFrame) { +TEST_F(VideoFrameCompositorTest, PaintFrameUsingOldRenderingPath) { scoped_refptr<VideoFrame> expected = VideoFrame::CreateEOSFrame(); // Should notify compositor synchronously. EXPECT_EQ(0, did_receive_frame_count()); - compositor()->UpdateCurrentFrame(expected); + compositor()->PaintFrameUsingOldRenderingPath(expected); scoped_refptr<VideoFrame> actual = compositor()->GetCurrentFrame(); EXPECT_EQ(expected, actual); EXPECT_EQ(1, did_receive_frame_count()); @@ -96,29 +101,29 @@ TEST_F(VideoFrameCompositorTest, NaturalSizeChanged) { EXPECT_EQ(0, natural_size_changed_count()); // Callback isn't fired for the first frame. - compositor()->UpdateCurrentFrame(initial_frame); + compositor()->PaintFrameUsingOldRenderingPath(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()->UpdateCurrentFrame(larger_frame); + compositor()->PaintFrameUsingOldRenderingPath(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()->UpdateCurrentFrame(larger_frame); + compositor()->PaintFrameUsingOldRenderingPath(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()->UpdateCurrentFrame(initial_frame); + compositor()->PaintFrameUsingOldRenderingPath(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()->UpdateCurrentFrame(initial_frame); + compositor()->PaintFrameUsingOldRenderingPath(initial_frame); EXPECT_EQ(initial_size.width(), natural_size().width()); EXPECT_EQ(initial_size, natural_size()); EXPECT_EQ(2, natural_size_changed_count()); @@ -137,22 +142,22 @@ TEST_F(VideoFrameCompositorTest, OpacityChanged) { EXPECT_EQ(0, opacity_changed_count()); // Callback is fired for the first frame. - compositor()->UpdateCurrentFrame(not_opaque_frame); + compositor()->PaintFrameUsingOldRenderingPath(not_opaque_frame); EXPECT_FALSE(opaque()); EXPECT_EQ(1, opacity_changed_count()); // Callback shouldn't be first subsequent times with same opaqueness. - compositor()->UpdateCurrentFrame(not_opaque_frame); + compositor()->PaintFrameUsingOldRenderingPath(not_opaque_frame); EXPECT_FALSE(opaque()); EXPECT_EQ(1, opacity_changed_count()); // Callback is fired when using opacity changes. - compositor()->UpdateCurrentFrame(opaque_frame); + compositor()->PaintFrameUsingOldRenderingPath(opaque_frame); EXPECT_TRUE(opaque()); EXPECT_EQ(2, opacity_changed_count()); // Callback shouldn't be first subsequent times with same opaqueness. - compositor()->UpdateCurrentFrame(opaque_frame); + compositor()->PaintFrameUsingOldRenderingPath(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 ab62772..8fa7a7f 100644 --- a/media/blink/webmediaplayer_impl.cc +++ b/media/blink/webmediaplayer_impl.cc @@ -133,8 +133,13 @@ WebMediaPlayerImpl::WebMediaPlayerImpl( context_3d_cb_(params.context_3d_cb()), supports_save_(true), chunk_demuxer_(NULL), - compositor_task_runner_(params.compositor_task_runner()), + // Threaded compositing isn't enabled universally yet. + compositor_task_runner_( + params.compositor_task_runner() + ? params.compositor_task_runner() + : base::MessageLoop::current()->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, @@ -144,10 +149,6 @@ 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)); @@ -902,14 +903,13 @@ void WebMediaPlayerImpl::StartPipeline() { pipeline_.Start( demuxer_.get(), - renderer_factory_->CreateRenderer(media_task_runner_, - audio_source_provider_.get()), + renderer_factory_->CreateRenderer( + media_task_runner_, audio_source_provider_.get(), compositor_), 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,21 +980,12 @@ 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->GetCurrentFrame(); + *video_frame_out = compositor->GetCurrentFrameAndUpdateIfStale(); event->Signal(); } diff --git a/media/media.gyp b/media/media.gyp index 47f8df3..272ee39 100644 --- a/media/media.gyp +++ b/media/media.gyp @@ -1667,44 +1667,6 @@ }, ], # 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 73ef9bf..2833222 100644 --- a/media/mojo/services/mojo_renderer_factory.cc +++ b/media/mojo/services/mojo_renderer_factory.cc @@ -21,7 +21,8 @@ MojoRendererFactory::~MojoRendererFactory() { scoped_ptr<Renderer> MojoRendererFactory::CreateRenderer( const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner, - AudioRendererSink* /* audio_renderer_sink */) { + AudioRendererSink* /* audio_renderer_sink */, + VideoRendererSink* /* video_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 2aa6cfc..944fae5 100644 --- a/media/mojo/services/mojo_renderer_factory.h +++ b/media/mojo/services/mojo_renderer_factory.h @@ -33,7 +33,8 @@ class MEDIA_EXPORT MojoRendererFactory : public RendererFactory { scoped_ptr<Renderer> CreateRenderer( const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner, - AudioRendererSink* audio_renderer_sink) final; + AudioRendererSink* audio_renderer_sink, + VideoRendererSink* video_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 e4a1332..d1b41a6 100644 --- a/media/mojo/services/mojo_renderer_impl.cc +++ b/media/mojo/services/mojo_renderer_impl.cc @@ -33,14 +33,12 @@ MojoRendererImpl::~MojoRendererImpl() { // Connection to |remote_media_renderer_| will error-out here. } -// TODO(xhwang): Support |paint_cb| and |waiting_for_decryption_key_cb|, -// if needed. +// TODO(xhwang): Support |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 338bcb8..cea3284 100644 --- a/media/mojo/services/mojo_renderer_impl.h +++ b/media/mojo/services/mojo_renderer_impl.h @@ -37,7 +37,6 @@ 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 7b6884d..0601c2c 100644 --- a/media/mojo/services/mojo_renderer_service.cc +++ b/media/mojo/services/mojo_renderer_service.cc @@ -14,6 +14,7 @@ #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" @@ -25,9 +26,6 @@ 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), @@ -40,6 +38,7 @@ 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(), @@ -49,7 +48,7 @@ MojoRendererService::MojoRendererService() renderer_config->GetAudioHardwareConfig(), media_log)); scoped_ptr<VideoRenderer> video_renderer(new VideoRendererImpl( - task_runner, + task_runner, video_renderer_sink_.get(), renderer_config->GetVideoDecoders(task_runner, base::Bind(&MediaLog::AddLogEvent, media_log)).Pass(), @@ -113,7 +112,6 @@ 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 2046e9d..6003515 100644 --- a/media/mojo/services/mojo_renderer_service.h +++ b/media/mojo/services/mojo_renderer_service.h @@ -27,6 +27,7 @@ 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. @@ -89,6 +90,7 @@ 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 311a260..5522da2 100644 --- a/media/mojo/services/renderer_config.cc +++ b/media/mojo/services/renderer_config.cc @@ -34,6 +34,10 @@ 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 08b7e0b..4a531d5 100644 --- a/media/mojo/services/renderer_config.h +++ b/media/mojo/services/renderer_config.h @@ -13,6 +13,7 @@ #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 { @@ -34,13 +35,13 @@ class PlatformRendererConfig { const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner, const LogCB& media_log_cb) = 0; - // The audio output sink used for rendering audio. + // The output sink used for rendering audio or video respectively. 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 { @@ -57,6 +58,7 @@ 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 66464a9..7cee1a7 100644 --- a/media/mojo/services/renderer_config_default.cc +++ b/media/mojo/services/renderer_config_default.cc @@ -24,6 +24,20 @@ 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() { @@ -87,6 +101,10 @@ 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 81162d3..0ec129a 100644 --- a/media/renderers/default_renderer_factory.cc +++ b/media/renderers/default_renderer_factory.cc @@ -38,7 +38,8 @@ 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) { + AudioRendererSink* audio_renderer_sink, + VideoRendererSink* video_renderer_sink) { DCHECK(audio_renderer_sink); // Create our audio decoders and renderer. @@ -75,8 +76,9 @@ 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_decoders.Pass(), true, media_log_)); + scoped_ptr<VideoRenderer> video_renderer( + new VideoRendererImpl(media_task_runner, video_renderer_sink, + 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 0827992..05cf2b1 100644 --- a/media/renderers/default_renderer_factory.h +++ b/media/renderers/default_renderer_factory.h @@ -15,6 +15,7 @@ class AudioHardwareConfig; class AudioRendererSink; class GpuVideoAcceleratorFactories; class MediaLog; +class VideoRendererSink; // The default factory class for creating RendererImpl. class MEDIA_EXPORT DefaultRendererFactory : public RendererFactory { @@ -27,7 +28,8 @@ class MEDIA_EXPORT DefaultRendererFactory : public RendererFactory { scoped_ptr<Renderer> CreateRenderer( const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner, - AudioRendererSink* audio_renderer_sink) final; + AudioRendererSink* audio_renderer_sink, + VideoRendererSink* video_renderer_sink) final; private: scoped_refptr<MediaLog> media_log_; diff --git a/media/renderers/renderer_impl.cc b/media/renderers/renderer_impl.cc index ac80216..62caf9d 100644 --- a/media/renderers/renderer_impl.cc +++ b/media/renderers/renderer_impl.cc @@ -81,7 +81,6 @@ 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) { @@ -91,7 +90,6 @@ 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) || @@ -100,7 +98,6 @@ 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; @@ -337,7 +334,6 @@ 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 feb6920..3f90271 100644 --- a/media/renderers/renderer_impl.h +++ b/media/renderers/renderer_impl.h @@ -48,7 +48,6 @@ 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; @@ -140,7 +139,6 @@ 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 75fc050..9fcefe4 100644 --- a/media/renderers/renderer_impl_unittest.cc +++ b/media/renderers/renderer_impl_unittest.cc @@ -49,7 +49,6 @@ 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: @@ -98,9 +97,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<6>(&video_ended_cb_), RunCallback<1>(status))); + SaveArg<5>(&video_ended_cb_), RunCallback<1>(status))); } void InitializeAndExpect(PipelineStatus start_status) { @@ -122,8 +121,6 @@ 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, @@ -475,10 +472,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<6>(&video_ended_cb_), + SaveArg<5>(&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 627ada6..3e0698f 100644 --- a/media/renderers/video_renderer_impl.cc +++ b/media/renderers/video_renderer_impl.cc @@ -22,10 +22,12 @@ 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), @@ -106,7 +108,6 @@ 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, @@ -118,7 +119,6 @@ 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,7 +131,8 @@ void VideoRendererImpl::Initialize( statistics_cb_ = statistics_cb; buffering_state_cb_ = buffering_state_cb; - paint_cb_ = paint_cb, + paint_cb_ = base::Bind(&VideoRendererSink::PaintFrameUsingOldRenderingPath, + base::Unretained(sink_)); ended_cb_ = ended_cb; error_cb_ = error_cb; wall_clock_time_cb_ = wall_clock_time_cb; @@ -143,6 +144,19 @@ 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 33938dc..7eb9814 100644 --- a/media/renderers/video_renderer_impl.h +++ b/media/renderers/video_renderer_impl.h @@ -21,6 +21,7 @@ #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 { @@ -36,6 +37,7 @@ 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. @@ -47,6 +49,7 @@ 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); @@ -58,7 +61,6 @@ 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, @@ -72,6 +74,11 @@ 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(); @@ -118,6 +125,8 @@ 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 df5c9d3..86f767f 100644 --- a/media/renderers/video_renderer_impl_unittest.cc +++ b/media/renderers/video_renderer_impl_unittest.cc @@ -54,6 +54,7 @@ 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_)); @@ -108,7 +109,6 @@ 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,9 +265,15 @@ class VideoRendererImplTest : public ::testing::Test { NiceMock<MockDemuxerStream> demuxer_stream_; // Use StrictMock<T> to catch missing/extra callbacks. - class MockCB { + // 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 { public: - MOCK_METHOD1(Display, void(const scoped_refptr<VideoFrame>&)); + MOCK_METHOD1(Start, void(VideoRendererSink::RenderCallback*)); + MOCK_METHOD0(Stop, void()); + MOCK_METHOD1(PaintFrameUsingOldRenderingPath, + void(const scoped_refptr<VideoFrame>&)); MOCK_METHOD1(BufferingStateChange, void(BufferingState)); }; StrictMock<MockCB> mock_cb_; @@ -349,7 +355,7 @@ TEST_F(VideoRendererImplTest, Initialize) { TEST_F(VideoRendererImplTest, InitializeAndStartPlayingFrom) { Initialize(); QueueFrames("0 10 20 30"); - EXPECT_CALL(mock_cb_, Display(HasTimestamp(0))); + EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(0))); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH)); StartPlayingFrom(0); Destroy(); @@ -363,7 +369,7 @@ TEST_F(VideoRendererImplTest, DestroyWhileInitializing) { TEST_F(VideoRendererImplTest, DestroyWhileFlushing) { Initialize(); QueueFrames("0 10 20 30"); - EXPECT_CALL(mock_cb_, Display(HasTimestamp(0))); + EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(0))); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH)); StartPlayingFrom(0); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_NOTHING)); @@ -374,7 +380,7 @@ TEST_F(VideoRendererImplTest, DestroyWhileFlushing) { TEST_F(VideoRendererImplTest, Play) { Initialize(); QueueFrames("0 10 20 30"); - EXPECT_CALL(mock_cb_, Display(HasTimestamp(0))); + EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(0))); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH)); StartPlayingFrom(0); Destroy(); @@ -393,7 +399,7 @@ TEST_F(VideoRendererImplTest, FlushWithNothingBuffered) { TEST_F(VideoRendererImplTest, DecodeError_Playing) { Initialize(); QueueFrames("0 10 20 30"); - EXPECT_CALL(mock_cb_, Display(HasTimestamp(0))); + EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(0))); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH)); StartPlayingFrom(0); @@ -414,7 +420,7 @@ TEST_F(VideoRendererImplTest, StartPlayingFrom_Exact) { Initialize(); QueueFrames("50 60 70 80 90"); - EXPECT_CALL(mock_cb_, Display(HasTimestamp(60))); + EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(60))); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH)); StartPlayingFrom(60); Destroy(); @@ -424,7 +430,7 @@ TEST_F(VideoRendererImplTest, StartPlayingFrom_RightBefore) { Initialize(); QueueFrames("50 60 70 80 90"); - EXPECT_CALL(mock_cb_, Display(HasTimestamp(50))); + EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(50))); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH)); StartPlayingFrom(59); Destroy(); @@ -434,7 +440,7 @@ TEST_F(VideoRendererImplTest, StartPlayingFrom_RightAfter) { Initialize(); QueueFrames("50 60 70 80 90"); - EXPECT_CALL(mock_cb_, Display(HasTimestamp(60))); + EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(60))); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH)); StartPlayingFrom(61); Destroy(); @@ -446,7 +452,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_, Display(HasTimestamp(0))); + EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(0))); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH)) .Times(AnyNumber()); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_NOTHING)) @@ -457,7 +463,7 @@ TEST_F(VideoRendererImplTest, StartPlayingFrom_LowDelay) { SatisfyPendingRead(); WaitableMessageLoopEvent event; - EXPECT_CALL(mock_cb_, Display(HasTimestamp(10))) + EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(10))) .WillOnce(RunClosure(event.GetClosure())); AdvanceTimeInMs(10); event.RunAndWait(); @@ -469,7 +475,7 @@ TEST_F(VideoRendererImplTest, StartPlayingFrom_LowDelay) { TEST_F(VideoRendererImplTest, DestroyDuringOutstandingRead) { Initialize(); QueueFrames("0 10 20 30"); - EXPECT_CALL(mock_cb_, Display(HasTimestamp(0))); + EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(0))); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH)); StartPlayingFrom(0); @@ -490,7 +496,7 @@ TEST_F(VideoRendererImplTest, Underflow) { { WaitableMessageLoopEvent event; - EXPECT_CALL(mock_cb_, Display(HasTimestamp(0))); + EXPECT_CALL(mock_cb_, PaintFrameUsingOldRenderingPath(HasTimestamp(0))); EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH)) .WillOnce(RunClosure(event.GetClosure())); StartPlayingFrom(0); @@ -503,16 +509,19 @@ TEST_F(VideoRendererImplTest, Underflow) { { SCOPED_TRACE("Waiting for frame drops"); WaitableMessageLoopEvent event; - 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))) + 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))) .WillOnce(RunClosure(event.GetClosure())); AdvanceTimeInMs(31); event.RunAndWait(); Mock::VerifyAndClearExpectations(&mock_cb_); } - // Advance time more, such that a new frame should have been displayed by now. + // Advance time more. Now we should signal having nothing. And put + // the last frame up for display. { 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 ead238c..6ce35f0 100644 --- a/media/test/pipeline_integration_test.cc +++ b/media/test/pipeline_integration_test.cc @@ -631,12 +631,8 @@ class PipelineIntegrationTestHost : public mojo::test::ApplicationTestBase, void SetUp() override { ApplicationTestBase::SetUp(); - - // TODO(dalecurtis): For some reason this isn't done... - if (!base::CommandLine::InitializedForCurrentProcess()) { - base::CommandLine::Init(0, NULL); + if (!IsMediaLibraryInitialized()) InitializeMediaLibraryForTesting(); - } } protected: @@ -681,8 +677,6 @@ 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, @@ -727,8 +721,6 @@ 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 da3a9f8..c17431a 100644 --- a/media/test/pipeline_integration_test_base.cc +++ b/media/test/pipeline_integration_test_base.cc @@ -26,6 +26,7 @@ using ::testing::_; using ::testing::AnyNumber; using ::testing::AtMost; +using ::testing::Invoke; using ::testing::InvokeWithoutArgs; using ::testing::SaveArg; @@ -34,6 +35,9 @@ 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), @@ -135,8 +139,6 @@ 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, @@ -238,9 +240,13 @@ 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(), + new VideoRendererImpl(message_loop_.message_loop_proxy(), &video_sink_, 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 c8ca915..aa41337 100644 --- a/media/test/pipeline_integration_test_base.h +++ b/media/test/pipeline_integration_test_base.h @@ -45,6 +45,20 @@ 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 & @@ -105,6 +119,7 @@ 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 deleted file mode 100644 index d09b6bf..0000000 --- a/media/tools/player_x11/data_source_logger.cc +++ /dev/null @@ -1,59 +0,0 @@ -// 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 deleted file mode 100644 index 13fdc60..0000000 --- a/media/tools/player_x11/data_source_logger.h +++ /dev/null @@ -1,41 +0,0 @@ -// 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 deleted file mode 100644 index 5f233c4..0000000 --- a/media/tools/player_x11/gl_video_renderer.cc +++ /dev/null @@ -1,251 +0,0 @@ -// 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 deleted file mode 100644 index a652eea..0000000 --- a/media/tools/player_x11/gl_video_renderer.h +++ /dev/null @@ -1,43 +0,0 @@ -// 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 deleted file mode 100644 index d149ca4..0000000 --- a/media/tools/player_x11/player_x11.cc +++ /dev/null @@ -1,311 +0,0 @@ -// 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 deleted file mode 100644 index 2ae8e3b..0000000 --- a/media/tools/player_x11/x11_video_renderer.cc +++ /dev/null @@ -1,215 +0,0 @@ -// 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 deleted file mode 100644 index 11213c3..0000000 --- a/media/tools/player_x11/x11_video_renderer.h +++ /dev/null @@ -1,47 +0,0 @@ -// 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 3bf1a36..1212519 100644 --- a/net/url_request/url_fetcher_impl_unittest.cc +++ b/net/url_request/url_fetcher_impl_unittest.cc @@ -200,6 +200,58 @@ 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_; } @@ -464,30 +516,6 @@ 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( @@ -637,44 +665,6 @@ 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. @@ -1202,149 +1192,93 @@ TEST_F(URLFetcherMultipleAttemptTest, SameData) { base::MessageLoop::current()->Run(); } -TEST_F(URLFetcherFileTest, SmallGet) { +// Get a small file. +TEST_F(URLFetcherTest, FileTestSmallGet) { + const char kFileToFetch[] = "simple.html"; + 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(). - - ASSERT_FALSE(base::PathExists(file_path_)) - << file_path_.value() << " not removed."; + base::FilePath out_path = temp_dir.path().AppendASCII(kFileToFetch); + SaveFileTest(kFileToFetch, false, out_path, false); } -TEST_F(URLFetcherFileTest, LargeGet) { +// 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"; + base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); - - // 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(). + base::FilePath out_path = temp_dir.path().AppendASCII(kFileToFetch); + SaveFileTest(kFileToFetch, false, out_path, false); } -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(); - } +// 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"; + + base::ScopedTempDir temp_dir; + ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); + base::FilePath out_path = temp_dir.path().AppendASCII(kFileToFetch); + SaveFileTest(kFileToFetch, false, out_path, true); } -TEST_F(URLFetcherFileTest, OverwriteExistingFile) { +// Test that an existing file can be overwritten be a fetcher. +TEST_F(URLFetcherTest, FileTestOverwriteExisting) { base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); // Create a file before trying to fetch. - static const char kFileToFetch[] = "simple.html"; + const char kFileToFetch[] = "simple.html"; std::string data(10000, '?'); // Meant to be larger than simple.html. - file_path_ = temp_dir.path().AppendASCII(kFileToFetch); + base::FilePath out_path = temp_dir.path().AppendASCII(kFileToFetch); ASSERT_EQ(static_cast<int>(data.size()), - 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_)); + base::WriteFile(out_path, data.data(), data.size())); + ASSERT_TRUE(base::PathExists(out_path)); - // Get a small file. - CreateFetcherForFile( - test_server_->GetURL(std::string(kTestServerFilePrefix) + kFileToFetch), - file_path_); - - base::MessageLoop::current()->Run(); // OnURLFetchComplete() will Quit(). + SaveFileTest(kFileToFetch, false, out_path, true); } -TEST_F(URLFetcherFileTest, TryToOverwriteDirectory) { +// Test trying to overwrite a directory with a file when using a fetcher fails. +TEST_F(URLFetcherTest, FileTestTryToOverwriteDirectory) { base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); // Create a directory before trying to fetch. static const char kFileToFetch[] = "simple.html"; - 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), - file_path_); + base::FilePath out_path = temp_dir.path().AppendASCII(kFileToFetch); + ASSERT_TRUE(base::CreateDirectory(out_path)); + ASSERT_TRUE(base::PathExists(out_path)); - base::MessageLoop::current()->Run(); // OnURLFetchComplete() will Quit(). + WaitingURLFetcherDelegate delegate; + delegate.CreateFetcherWithContext( + 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(); - base::MessageLoop::current()->RunUntilIdle(); + EXPECT_FALSE(delegate.fetcher()->GetStatus().is_success()); + EXPECT_EQ(ERR_ACCESS_DENIED, delegate.fetcher()->GetStatus().error()); } -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 small file and save it to a temp file. +TEST_F(URLFetcherTest, TempFileTestSmallGet) { + SaveFileTest("simple.html", 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(). +// 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, 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(); - } +// 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); } } // namespace diff --git a/net/url_request/url_request_test_util.h b/net/url_request/url_request_test_util.h index 0208b43..c856c98 100644 --- a/net/url_request/url_request_test_util.h +++ b/net/url_request/url_request_test_util.h @@ -70,7 +70,8 @@ class TestURLRequestContext : public URLRequestContext { } void set_http_network_session_params( - const HttpNetworkSession::Params& params) { + scoped_ptr<HttpNetworkSession::Params> params) { + http_network_session_params_ = params.Pass(); } void SetSdchManager(scoped_ptr<SdchManager> sdch_manager) { diff --git a/sync/BUILD.gn b/sync/BUILD.gn index cd6e0e4..9a95bb1 100644 --- a/sync/BUILD.gn +++ b/sync/BUILD.gn @@ -819,6 +819,7 @@ 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 cc8fe81..79c0bab 100644 --- a/sync/test/fake_server/android/fake_server_helper_android.cc +++ b/sync/test/fake_server/android/fake_server_helper_android.cc @@ -14,11 +14,14 @@ #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) { } @@ -105,6 +108,44 @@ 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 d7449d7..11f2234 100644 --- a/sync/test/fake_server/android/fake_server_helper_android.h +++ b/sync/test/fake_server/android/fake_server_helper_android.h @@ -7,6 +7,7 @@ #include <jni.h> +#include "base/android/scoped_java_ref.h" #include "base/basictypes.h" #include "sync/test/fake_server/entity_builder_factory.h" @@ -47,6 +48,20 @@ 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 5abad58..aabc7bb 100644 --- a/sync/test/fake_server/bookmark_entity_builder.cc +++ b/sync/test/fake_server/bookmark_entity_builder.cc @@ -45,6 +45,10 @@ 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); @@ -62,6 +66,10 @@ 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, @@ -72,10 +80,7 @@ scoped_ptr<FakeServerEntity> BookmarkEntityBuilder::Build() { entity_specifics, // TODO(pvalenzuela): Support bookmark folders. false, - // TODO(pvalenzuela): Support caller specification of - // the parent bookmark folder. - FakeServerEntity::CreateId(syncer::BOOKMARKS, - "bookmark_bar"), + parent_id_, kDefaultTime, kDefaultTime)); } diff --git a/sync/test/fake_server/bookmark_entity_builder.h b/sync/test/fake_server/bookmark_entity_builder.h index 62c7169..b7f897e0 100644 --- a/sync/test/fake_server/bookmark_entity_builder.h +++ b/sync/test/fake_server/bookmark_entity_builder.h @@ -25,6 +25,10 @@ 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; @@ -33,6 +37,9 @@ 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 c54a9e9..dbe3105 100644 --- a/sync/test/fake_server/fake_server.cc +++ b/sync/test/fake_server/fake_server.cc @@ -34,17 +34,29 @@ 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"; -namespace fake_server { +// Properties of the bookmark bar permanent folder. +static const char kBookmarkBarFolderServerTag[] = "bookmark_bar"; +static const char kBookmarkBarFolderName[] = "Bookmark Bar"; -class FakeServerEntity; +// Properties of the other bookmarks permanent folder. +static const char kOtherBookmarksFolderServerTag[] = "other_bookmarks"; +static const char kOtherBookmarksFolderName[] = "Other Bookmarks"; -namespace { +// Properties of the synced bookmarks permanent folder. +static const char kSyncedBookmarksFolderServerTag[] = "synced_bookmarks"; +static const char kSyncedBookmarksFolderName[] = "Synced Bookmarks"; // 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 @@ -167,8 +179,8 @@ FakeServer::~FakeServer() { STLDeleteContainerPairSecondPointers(entities_.begin(), entities_.end()); } -bool FakeServer::CreatePermanentBookmarkFolder(const char* server_tag, - const char* name) { +bool FakeServer::CreatePermanentBookmarkFolder(const std::string& server_tag, + const std::string& name) { FakeServerEntity* entity = PermanentEntity::Create(syncer::BOOKMARKS, server_tag, name, ModelTypeToRootTag(syncer::BOOKMARKS)); @@ -191,9 +203,11 @@ bool FakeServer::CreateDefaultPermanentItems() { SaveEntity(top_level_entity); if (model_type == syncer::BOOKMARKS) { - if (!CreatePermanentBookmarkFolder("bookmark_bar", "Bookmark Bar")) + if (!CreatePermanentBookmarkFolder(kBookmarkBarFolderServerTag, + kBookmarkBarFolderName)) return false; - if (!CreatePermanentBookmarkFolder("other_bookmarks", "Other Bookmarks")) + if (!CreatePermanentBookmarkFolder(kOtherBookmarksFolderServerTag, + kOtherBookmarksFolderName)) return false; } } @@ -201,21 +215,6 @@ 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_); @@ -291,8 +290,11 @@ 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() && - !CreateMobileBookmarksPermanentItem()) { + !CreatePermanentBookmarkFolder(kSyncedBookmarksFolderServerTag, + kSyncedBookmarksFolderName)) { return false; } @@ -601,4 +603,18 @@ 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 78edd9a..ecc8bd7 100644 --- a/sync/test/fake_server/fake_server.h +++ b/sync/test/fake_server/fake_server.h @@ -116,6 +116,9 @@ 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; @@ -128,14 +131,13 @@ class FakeServer { const std::string& invalidator_client_id, sync_pb::CommitResponse* response); - bool CreatePermanentBookmarkFolder(const char* server_tag, const char* name); + // Creates and saves a permanent folder for Bookmarks (e.g., Bookmark Bar). + bool CreatePermanentBookmarkFolder(const std::string& server_tag, + const std::string& 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 7af21da..53bbf03 100644 --- a/tools/mb/mb_config.pyl +++ b/tools/mb/mb_config.pyl @@ -22,8 +22,10 @@ '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_bot_win_x86': ['gn', 'debug_static_bot', 'x86'], + 'gn_debug_static_bot': ['gn', 'debug_static_bot'], + 'gn_debug_static_bot_x86': ['gn', 'debug_static_bot', 'x86'], 'gyp_release_bot': ['gyp', 'release_bot'], }, @@ -145,11 +147,11 @@ }, 'chromium.mac': { 'Mac GN': 'gn_release_bot', - 'Mac GN (dbg)': 'gn_debug_bot', + 'Mac GN (dbg)': 'gn_debug_static_bot', }, 'chromium.win': { 'Win8 GN': 'gn_release_bot_x86', - 'Win8 GN (dbg)': 'gn_debug_bot_win_x86', + 'Win8 GN (dbg)': 'gn_debug_static_bot_x86', }, 'chromium.webkit': { 'Android GN': 'gn_release_bot', @@ -172,13 +174,13 @@ 'linux_chromium_gn_upload_x86': 'gn_release_bot_x86', }, 'tryserver.chromium.mac': { - 'mac_chromium_gn_dbg': 'gn_debug_bot', + 'mac_chromium_gn_dbg': 'gn_debug_static_bot', 'mac_chromium_gn_rel': 'gn_release_trybot', 'mac_chromium_gn_upload': 'gn_release_bot', }, 'tryserver.chromium.win': { - 'win8_chromium_gn_dbg': 'gn_debug_bot', - 'win8_chromium_gn_rel': 'gn_release_trybot', + 'win8_chromium_gn_dbg': 'gn_debug_static_bot_x86', + 'win8_chromium_gn_rel': 'gn_release_trybot_x86', 'win8_chromium_gn_upload': 'gn_release_bot', }, }, diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index f53c021..eee68bd 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -24274,6 +24274,15 @@ 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 dda359b..a9c7e54 100755 --- a/tools/run-bisect-perf-regression.py +++ b/tools/run-bisect-perf-regression.py @@ -37,6 +37,7 @@ 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' @@ -634,7 +635,8 @@ def _GetAffectedBenchmarkModuleNames(): all_affected_files = _GetModifiedFilesFromPatch() modified_benchmarks = [] for affected_file in all_affected_files: - if affected_file.startswith(PERF_BENCHMARKS_PATH): + if (affected_file.startswith(PERF_BENCHMARKS_PATH) or + affected_file.startswith(PERF_MEASUREMENTS_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 34c11e3..3e61d40 100644 --- a/ui/compositor/layer.cc +++ b/ui/compositor/layer.cc @@ -751,11 +751,11 @@ void Layer::PaintContents( delegate_->OnPaintLayer(PaintContext(canvas.get(), clip)); } -scoped_refptr<cc::DisplayItemList> Layer::PaintContentsToDisplayList( +void Layer::PaintContentsToDisplayList( + cc::DisplayItemList* display_list, 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,9 +763,8 @@ scoped_refptr<cc::DisplayItemList> Layer::PaintContentsToDisplayList( gfx::Rect invalidation = clip; DCHECK(clip.Contains(invalidation)); delegate_->OnPaintLayer( - PaintContext(list.get(), device_scale_factor_, clip, invalidation)); + PaintContext(display_list, 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 3dbd173..381e982 100644 --- a/ui/compositor/layer.h +++ b/ui/compositor/layer.h @@ -342,7 +342,8 @@ class COMPOSITOR_EXPORT Layer SkCanvas* canvas, const gfx::Rect& clip, ContentLayerClient::PaintingControlSetting painting_control) override; - scoped_refptr<cc::DisplayItemList> PaintContentsToDisplayList( + void PaintContentsToDisplayList( + cc::DisplayItemList* display_list, 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 37e16eb..8e4c5812 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,3 +63,20 @@ 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 8f99f24..b1a5b85 100644 --- a/ui/file_manager/file_manager/foreground/js/compiled_resources.gyp +++ b/ui/file_manager/file_manager/foreground/js/compiled_resources.gyp @@ -114,6 +114,7 @@ './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 46183c7..45548c1 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,6 +67,8 @@ 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} @@ -190,6 +192,13 @@ 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); } /** @@ -519,8 +528,7 @@ CWSWidgetContainer.prototype.onItemInstalled_ = function(result, error) { case AppInstaller.Result.ERROR: CWSWidgetContainer.Metrics.recordInstall( CWSWidgetContainer.Metrics.INSTALL.FAILED); - // TODO(tbarzic): Remove dialog showing call from this class. - fileManager.ui.errorDialog.show( + this.errorDialog_.show( str('SUGGEST_DIALOG_INSTALLATION_FAILED'), null, null, @@ -644,6 +652,9 @@ 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 04b8628..c078f95 100644 --- a/ui/file_manager/file_manager/foreground/js/main_scripts.js +++ b/ui/file_manager/file_manager/foreground/js/main_scripts.js @@ -132,6 +132,7 @@ //<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 new file mode 100644 index 0000000..28efbb4 --- /dev/null +++ b/ui/file_manager/file_manager/foreground/js/ui/cws_widget_container_error_dialog.js @@ -0,0 +1,58 @@ +// 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 4cc1490..a4ae31d 100644 --- a/ui/file_manager/file_manager/main.html +++ b/ui/file_manager/file_manager/main.html @@ -155,6 +155,7 @@ <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> |