Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2801823003:
[Presentation API] Change connection to 'connected' if start a presentation with "https://www.googl… (Closed)
Description[Presentation API] Change connection to 'connected' if start a presentation with "https://www.google.com"
- always create navigator.presentation.receiver object for receiver page
- create receiver PSImpl in PresentationDispatcher::DidFinishLoadDocument()
- do not reset() if receiver page navigates from blank to https://www.google.com
BUG=693309
Review-Url: https://codereview.chromium.org/2801823003
Cr-Commit-Position: refs/heads/master@{#463820}
Committed: https://chromium.googlesource.com/chromium/src/+/cae9c010f6e47a9b306f10b8889f86a65b6e9cb0
Patch Set 1 #Patch Set 2 : fix layout test #Patch Set 3 : fix content_unittest and layout test failures #
Total comments: 4
Patch Set 4 : resolve code review comments from Derek #
Total comments: 8
Patch Set 5 : resolve code review comments from mlamouri #
Total comments: 6
Patch Set 6 : resolve code review comments from Mark #
Total comments: 2
Patch Set 7 : resolve code review comments from Derek #
Total comments: 3
Patch Set 8 : resolve code review comments from mlamouri #Patch Set 9 : merge with master #Patch Set 10 : fix windows compile error #
Messages
Total messages: 52 (33 generated)
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
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: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
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.
zhaobin@chromium.org changed reviewers: + imcheng@chromium.org, mfoltz@chromium.org, mlamouri@chromium.org
Looks good but I didn't look in depth into the Blink side. Can you clarify whether this is an issue with the receiver frame navigating to "google.com" or just navigations in general? https://codereview.chromium.org/2801823003/diff/40001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/2801823003/diff/40001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:420: if (receiver_delegate_) Please add a comment here. https://codereview.chromium.org/2801823003/diff/40001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2801823003/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:388: if (!render_frame() || !render_frame()->GetWebFrame()) This seems a bit easier to understand if you flipped the condition and combined with the case below, but I don't feel strongly.
https://codereview.chromium.org/2801823003/diff/40001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/2801823003/diff/40001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:420: if (receiver_delegate_) On 2017/04/08 00:05:59, imcheng wrote: > Please add a comment here. Done. https://codereview.chromium.org/2801823003/diff/40001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2801823003/diff/40001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:388: if (!render_frame() || !render_frame()->GetWebFrame()) On 2017/04/08 00:05:59, imcheng wrote: > This seems a bit easier to understand if you flipped the condition and combined > with the case below, but I don't feel strongly. Done.
Thanks for the CL :) In addition of the comments below, I would recommend using https://example.com for instead of google.com when using an URL as an example as google.com might sound like special-casing. https://codereview.chromium.org/2801823003/diff/60001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/2801823003/diff/60001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:423: if (receiver_delegate_) How do we know the navigation came from blank here? https://codereview.chromium.org/2801823003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/presentation/NavigatorPresentation.cpp (right): https://codereview.chromium.org/2801823003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/presentation/NavigatorPresentation.cpp:37: supplement->m_presentation = Presentation::create(navigator.frame()); This seems to duplicate the logic in `NavigatorPresentation::presentation()`. Do you do this so when LocalFrameClientImpl gets the NavigatorPresentation, it will create the receiver object? Would it make sense to have a method that make this clear such as `ensurePresentationReceiver`? https://codereview.chromium.org/2801823003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/presentation/NavigatorPresentation.h (right): https://codereview.chromium.org/2801823003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/presentation/NavigatorPresentation.h:19: class MODULES_EXPORT NavigatorPresentation final Maybe just add MODULES_EXPORT on the method that needs it? https://codereview.chromium.org/2801823003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/presentation/Presentation.cpp (right): https://codereview.chromium.org/2801823003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/presentation/Presentation.cpp:28: if (frame->settings() && frame->settings()->getPresentationReceiver()) { Maybe add a comment explaining that this need to be eagerly created in order to have the receiver associated with the client?
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/2801823003/diff/60001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/2801823003/diff/60001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:423: if (receiver_delegate_) On 2017/04/10 12:50:28, mlamouri wrote: > How do we know the navigation came from blank here? Code removed. https://codereview.chromium.org/2801823003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/presentation/NavigatorPresentation.cpp (right): https://codereview.chromium.org/2801823003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/presentation/NavigatorPresentation.cpp:37: supplement->m_presentation = Presentation::create(navigator.frame()); On 2017/04/10 12:50:28, mlamouri wrote: > This seems to duplicate the logic in `NavigatorPresentation::presentation()`. Do > you do this so when LocalFrameClientImpl gets the NavigatorPresentation, it will > create the receiver object? Would it make sense to have a method that make this > clear such as `ensurePresentationReceiver`? Done. https://codereview.chromium.org/2801823003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/presentation/NavigatorPresentation.h (right): https://codereview.chromium.org/2801823003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/presentation/NavigatorPresentation.h:19: class MODULES_EXPORT NavigatorPresentation final On 2017/04/10 12:50:28, mlamouri wrote: > Maybe just add MODULES_EXPORT on the method that needs it? Done. https://codereview.chromium.org/2801823003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/presentation/Presentation.cpp (right): https://codereview.chromium.org/2801823003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/presentation/Presentation.cpp:28: if (frame->settings() && frame->settings()->getPresentationReceiver()) { On 2017/04/10 12:50:28, mlamouri wrote: > Maybe add a comment explaining that this need to be eagerly created in order to > have the receiver associated with the client? Code removed.
Thanks for finding a fix. It looks like this is a common pattern for Web APIs that require eager initialization at load time. Important that anything with side effects only happens if the page is a presentation receiver. https://codereview.chromium.org/2801823003/diff/100001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2801823003/diff/100001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:408: void PresentationDispatcher::DidFinishDocumentLoad() { I'm assuming that receiver_ is set only for presentation receiver documents; we don't want to connect to the presentation service in other documents. https://codereview.chromium.org/2801823003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/presentation/NavigatorPresentation.cpp (right): https://codereview.chromium.org/2801823003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/presentation/NavigatorPresentation.cpp:23: NavigatorPresentation* NavigatorPresentation::ensurePresentationReceiver( Slight preference to move this to PresentationReceiver, i.e. PresentationReceiver::from(Document& document) or PresentationReceiver::from(Navigator& navigator) so the code in LocalFrameClientImpl looks similar to the other APIs that require eager initialization. https://codereview.chromium.org/2801823003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/presentation/NavigatorPresentation.cpp:26: return 0; nullptr
Thanks for finding a fix. It looks like this is a common pattern for Web APIs that require eager initialization at load time. Important that anything with side effects only happens if the page is a presentation receiver.
lgtm
https://codereview.chromium.org/2801823003/diff/100001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/2801823003/diff/100001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:408: void PresentationDispatcher::DidFinishDocumentLoad() { On 2017/04/10 21:59:57, mark a. foltz wrote: > I'm assuming that receiver_ is set only for presentation receiver documents; we > don't want to connect to the presentation service in other documents. Yes. receiver_ is only set for receiver documents. https://codereview.chromium.org/2801823003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/presentation/NavigatorPresentation.cpp (right): https://codereview.chromium.org/2801823003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/presentation/NavigatorPresentation.cpp:23: NavigatorPresentation* NavigatorPresentation::ensurePresentationReceiver( On 2017/04/10 21:59:57, mark a. foltz wrote: > Slight preference to move this to PresentationReceiver, i.e. > > PresentationReceiver::from(Document& document) > > or > > PresentationReceiver::from(Navigator& navigator) > > > so the code in LocalFrameClientImpl looks similar to the other APIs that require > eager initialization. Done. https://codereview.chromium.org/2801823003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/presentation/NavigatorPresentation.cpp:26: return 0; On 2017/04/10 21:59:57, mark a. foltz wrote: > nullptr Done.
lgtm https://codereview.chromium.org/2801823003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/presentation/PresentationReceiver.h (right): https://codereview.chromium.org/2801823003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/presentation/PresentationReceiver.h:43: // This need to be eagerly created in order to have the receiver associated nit: Feels like this should be a class-level comment? I don't feel strongly though.
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #7 (id:140001) has been deleted
zhaobin@chromium.org changed reviewers: + haraken@chromium.org
+haraken@ to review: third_party/WebKit/Source/web/LocalFrameClientImpl.cpp https://codereview.chromium.org/2801823003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/presentation/PresentationReceiver.h (right): https://codereview.chromium.org/2801823003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/presentation/PresentationReceiver.h:43: // This need to be eagerly created in order to have the receiver associated On 2017/04/11 02:00:27, imcheng wrote: > nit: Feels like this should be a class-level comment? I don't feel strongly > though. Done.
LGTM (Though I think the random initialization code in LocalFrameClientImpl::dispatchDidClearWindowObjectInMainWorld should be moved to a more appropriate place.)
lgtm with comment addressed. https://codereview.chromium.org/2801823003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/LocalFrameClientImpl.cpp (right): https://codereview.chromium.org/2801823003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/LocalFrameClientImpl.cpp:166: PresentationReceiver::from(*document); nit: can you either add a comment explaining that you call this in order to ensure the object is created or rename the method something else than from? Because the method is only used in this context and it returns a PresentationReceiver* which is never actually used which can sound very weird to a reader, I would prefer a more explicit name but I will leave this up to you :)
https://codereview.chromium.org/2801823003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/LocalFrameClientImpl.cpp (right): https://codereview.chromium.org/2801823003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/LocalFrameClientImpl.cpp:166: PresentationReceiver::from(*document); On 2017/04/11 at 13:10:16, mlamouri wrote: > nit: can you either add a comment explaining that you call this in order to ensure the object is created or rename the method something else than from? Because the method is only used in this context and it returns a PresentationReceiver* which is never actually used which can sound very weird to a reader, I would prefer a more explicit name but I will leave this up to you :) I asked Bin to implement this to be consistent with the surrounding code (for better or worse). I agree that a comment would help explain the intent. Perhaps all of these from() methods should be renamed to createFor() or allocateFor() to make the side effect more obvious.
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2801823003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/LocalFrameClientImpl.cpp (right): https://codereview.chromium.org/2801823003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/LocalFrameClientImpl.cpp:166: PresentationReceiver::from(*document); On 2017/04/11 13:10:16, mlamouri wrote: > nit: can you either add a comment explaining that you call this in order to > ensure the object is created or rename the method something else than from? > Because the method is only used in this context and it returns a > PresentationReceiver* which is never actually used which can sound very weird to > a reader, I would prefer a more explicit name but I will leave this up to you :) Added a comment :) Mark suggested naming the function as PresentationReceiver::from() to be consistent with other APIs above. https://codereview.chromium.org/2801823003/diff/100001/third_party/WebKit/Sou...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
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: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
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 zhaobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, imcheng@chromium.org, haraken@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2801823003/#ps220001 (title: "fix windows compile error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1491947273281580,
"parent_rev": "42dcba91524903cd72eac79865d7fd0b3d23bee6", "commit_rev":
"91c6a0fd033e392b1cd1fe4130946a9cde4d79df"}
The CQ bit was unchecked by commit-bot@chromium.org
Prior attempt to commit was detected, but we were not able to check whether the issue was successfully committed. Please check Git history manually and re-check CQ or close this issue as needed.
The CQ bit was checked by zhaobin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1491951237300990,
"parent_rev": "874510b10809e076102e0327be11c55e6019ea37", "commit_rev":
"cae9c010f6e47a9b306f10b8889f86a65b6e9cb0"}
Message was sent while issue was closed.
Description was changed from ========== [Presentation API] Change connection to 'connected' if start a presentation with "https://www.google.com" - always create navigator.presentation.receiver object for receiver page - create receiver PSImpl in PresentationDispatcher::DidFinishLoadDocument() - do not reset() if receiver page navigates from blank to https://www.google.com BUG=693309 ========== to ========== [Presentation API] Change connection to 'connected' if start a presentation with "https://www.google.com" - always create navigator.presentation.receiver object for receiver page - create receiver PSImpl in PresentationDispatcher::DidFinishLoadDocument() - do not reset() if receiver page navigates from blank to https://www.google.com BUG=693309 Review-Url: https://codereview.chromium.org/2801823003 Cr-Commit-Position: refs/heads/master@{#463820} Committed: https://chromium.googlesource.com/chromium/src/+/cae9c010f6e47a9b306f10b8889f... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:220001) as https://chromium.googlesource.com/chromium/src/+/cae9c010f6e47a9b306f10b8889f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
