Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2338173005:
[Blink, RemotePlayback] Dismissing the dialog rejects with NotAllowedError. (Closed)
|
Created:
4 years, 3 months ago by whywhat Modified:
4 years, 2 months ago Reviewers:
mlamouri (slow - plz ping) CC:
blink-reviews, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Blink, RemotePlayback] Dismissing the dialog rejects with NotAllowedError.
BUG=647441
TEST=manual
Committed: https://crrev.com/1cf2282189036144048ff9233d31f2d2fc55eadf
Cr-Commit-Position: refs/heads/master@{#423711}
Patch Set 1 #Patch Set 2 : Added a unit test #Patch Set 3 : Added modules export to RemotePlayback #
Total comments: 8
Patch Set 4 : Fixed the unit test #Patch Set 5 : Removed a ; #Patch Set 6 : Fixed base class exports for WIN build #
Total comments: 4
Patch Set 7 : NON_EXPORTED_BASE #
Depends on Patchset: Messages
Total messages: 37 (14 generated)
avayvod@chromium.org changed reviewers: + mlamouri@chromium.org
PTaL
Can we add a layout test?
On 2016/09/16 at 01:29:42, haraken wrote:
> Can we add a layout test?
Hm, unless we introduce some
internals.settings.setRemotePlaybackDialogWhenPrompt('dismiss') or something,
I'm not sure. The dialog shown on Android is native and lives in a browser
process so it's not easily dismissable from a LayoutTest.
Would a unit test be possible then?
Added a unit test
On 2016/09/22 at 09:25:00, mlamouri wrote: > Would a unit test be possible then? I've added a unit test but it doesn't link locally (can't find RemotePlayback methods? should these be exported somehow?)
Added modules export to RemotePlayback
lgtm with test changes https://codereview.chromium.org/2338173005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp (right): https://codereview.chromium.org/2338173005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp:36: }; Don't think this ; is needed. https://codereview.chromium.org/2338173005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp:42: auto documentBody = pageHolder->document().body(); style: I'm not 100% sure this is style-required but it sounds like an abuse of 'auto'. Can you just write the type names? https://codereview.chromium.org/2338173005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp:46: HTMLMediaElement* element = toHTMLMediaElement(documentBody->firstChild()); Instead of adding a video element to the document via parsing, why not: ``` HTMLMediaElement* element = HTMLVideoElement::create(pageHolder->document()); ``` https://codereview.chromium.org/2338173005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp:47: auto remotePlayback = RemotePlayback::create(*element); ditto
Fixed the unit test
Removed a ;
https://codereview.chromium.org/2338173005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp (right): https://codereview.chromium.org/2338173005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp:36: }; On 2016/10/01 at 14:56:30, mlamouri wrote: > Don't think this ; is needed. Done. https://codereview.chromium.org/2338173005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp:42: auto documentBody = pageHolder->document().body(); On 2016/10/01 at 14:56:30, mlamouri wrote: > style: I'm not 100% sure this is style-required but it sounds like an abuse of 'auto'. Can you just write the type names? DummyPageHolder::create seems fine (returns a smart pointer, auto is used elsewhere). Removed the rest. https://codereview.chromium.org/2338173005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp:46: HTMLMediaElement* element = toHTMLMediaElement(documentBody->firstChild()); On 2016/10/01 at 14:56:30, mlamouri wrote: > Instead of adding a video element to the document via parsing, why not: > ``` > HTMLMediaElement* element = HTMLVideoElement::create(pageHolder->document()); > ``` Yep, that's better. https://codereview.chromium.org/2338173005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp:47: auto remotePlayback = RemotePlayback::create(*element); On 2016/10/01 at 14:56:30, mlamouri wrote: > ditto Done.
The CQ bit was checked by avayvod@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2338173005/#ps80001 (title: "Removed a ;")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Fixed base class exports for WIN build
The CQ bit was checked by avayvod@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2338173005/#ps100001 (title: "Fixed base class exports for WIN build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
https://codereview.chromium.org/2338173005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h (right): https://codereview.chromium.org/2338173005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h:28: private WebRemotePlaybackClient { should you do `NON_EXPORTED_BASE(WebRemotePlaybackClient)` to solve the Windows build?
https://codereview.chromium.org/2338173005/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/remoteplayback/WebRemotePlaybackClient.h (right): https://codereview.chromium.org/2338173005/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/remoteplayback/WebRemotePlaybackClient.h:16: class BLINK_PLATFORM_EXPORT WebRemotePlaybackClient { Just saw that you did this. Why exporting something we don't need to export?
NON_EXPORTED_BASE
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
https://codereview.chromium.org/2338173005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h (right): https://codereview.chromium.org/2338173005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h:28: private WebRemotePlaybackClient { On 2016/10/06 at 10:55:08, mlamouri wrote: > should you do `NON_EXPORTED_BASE(WebRemotePlaybackClient)` to solve the Windows build? I should probably :P https://codereview.chromium.org/2338173005/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/remoteplayback/WebRemotePlaybackClient.h (right): https://codereview.chromium.org/2338173005/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/remoteplayback/WebRemotePlaybackClient.h:16: class BLINK_PLATFORM_EXPORT WebRemotePlaybackClient { On 2016/10/06 at 10:55:38, mlamouri wrote: > Just saw that you did this. Why exporting something we don't need to export? Because I didn't remember NON_EXPORTED_BASE ?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by avayvod@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2338173005/#ps120001 (title: "NON_EXPORTED_BASE")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Blink, RemotePlayback] Dismissing the dialog rejects with NotAllowedError. BUG=647441 TEST=manual ========== to ========== [Blink, RemotePlayback] Dismissing the dialog rejects with NotAllowedError. BUG=647441 TEST=manual Committed: https://crrev.com/1cf2282189036144048ff9233d31f2d2fc55eadf Cr-Commit-Position: refs/heads/master@{#423711} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/1cf2282189036144048ff9233d31f2d2fc55eadf Cr-Commit-Position: refs/heads/master@{#423711} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
