Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2461503002:
Provide URL-related info to the VR HTML UI. (Closed)
DescriptionProvide URL-related info to the VR HTML UI.
This lets UI render a basic URL bar.
BUG=641508
Committed: https://crrev.com/f0cecd7df4cb20f9c5302d44da3547de4895a521
Cr-Commit-Position: refs/heads/master@{#428621}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Improvements + nits. #Patch Set 3 : Rebase #Patch Set 4 : Rebase further ahead to resolve conflict. #
Messages
Total messages: 28 (10 generated)
cjgrant@chromium.org changed reviewers: + bshe@chromium.org, mthiesse@chromium.org
https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/ui_interface.cc (right): https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/ui_interface.cc:51: handler_ = handler; Is the diff suppose to be here? The function was defined in header file but not implemented before your patch? https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_web_contents_observer.cc (right): https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_web_contents_observer.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2016 https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_web_contents_observer.cc:31: ui_interface_->SetURL(navigation_handle->GetURL()); Not sure if redirect URL works? https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_web_contents_observer.h (right): https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_web_contents_observer.h:22: explicit VrWebContentsObserver(content::WebContents* web_contents, explicit is not needed for ctor that has two parameters
lgtm
https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_web_contents_observer.h (right): https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_web_contents_observer.h:23: UiInterface *ui_interface); nit: * on type https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_web_contents_observer.h:26: void SetUiInterface(UiInterface *ui_interface); nit: * on type
https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_web_contents_observer.h (right): https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_web_contents_observer.h:23: UiInterface *ui_interface); nit: * on type https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_web_contents_observer.h:26: void SetUiInterface(UiInterface *ui_interface); nit: * on type
https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_web_contents_observer.cc (right): https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_web_contents_observer.cc:26: void VrWebContentsObserver::DidStopLoading() { nit: newline here and below.
https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/ui_interface.cc (right): https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/ui_interface.cc:51: handler_ = handler; On 2016/10/27 20:27:44, bshe wrote: > Is the diff suppose to be here? The function was defined in header file but not > implemented before your patch? This was intentional. I'm just moving the function to match the (better) order in the header file. https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_web_contents_observer.cc (right): https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_web_contents_observer.cc:26: void VrWebContentsObserver::DidStopLoading() { On 2016/10/27 21:31:17, mthiesse wrote: > nit: newline here and below. Done. https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_web_contents_observer.cc:31: ui_interface_->SetURL(navigation_handle->GetURL()); On 2016/10/27 20:27:44, bshe wrote: > Not sure if redirect URL works? You were right, this was incomplete. I now cover all three navigation callbacks. I used a test page with a redirect to verify that we now see the following callbacks: Start: http://wikipedia.com/ Redirect: http://www.wikipedia.org/ Redirect: https://www.wikipedia.org/ Finish: https://www.wikipedia.org/ Much better. https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_web_contents_observer.h (right): https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_web_contents_observer.h:22: explicit VrWebContentsObserver(content::WebContents* web_contents, On 2016/10/27 20:27:44, bshe wrote: > explicit is not needed for ctor that has two parameters Done. https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_web_contents_observer.h:23: UiInterface *ui_interface); On 2016/10/27 21:21:07, mthiesse wrote: > nit: * on type Done. https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_web_contents_observer.h:26: void SetUiInterface(UiInterface *ui_interface); On 2016/10/27 21:21:07, mthiesse wrote: > nit: * on type Done.
Also, for the record, I hope much of this code ends up being temporary. I would much rather we reuse existing omnibox code to observe navigation, and refactor it if necessary to let it drive our omnibox. Once we get to demo state, this will get revisited.
lgtm https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/ui_interface.cc (right): https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/ui_interface.cc:51: handler_ = handler; On 2016/10/28 14:16:58, cjgrant wrote: > On 2016/10/27 20:27:44, bshe wrote: > > Is the diff suppose to be here? The function was defined in header file but > not > > implemented before your patch? > > This was intentional. I'm just moving the function to match the (better) order > in the header file. hah. I missed the deleted function.. https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_web_contents_observer.cc (right): https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_web_contents_observer.cc:31: ui_interface_->SetURL(navigation_handle->GetURL()); On 2016/10/28 14:16:59, cjgrant wrote: > On 2016/10/27 20:27:44, bshe wrote: > > Not sure if redirect URL works? > > You were right, this was incomplete. I now cover all three navigation > callbacks. I used a test page with a redirect to verify that we now see the > following callbacks: > > Start: http://wikipedia.com/ > Redirect: http://www.wikipedia.org/ > Redirect: https://www.wikipedia.org/ > Finish: https://www.wikipedia.org/ > > Much better. > Do you need to update intermediate urls? It looks like Clank's ominibox only shows first and last url? Or perhaps it flashes too fast and I can't see. But in any case, would be nice to keep the behavior the same as Clank, whatever Clank use.
On 2016/10/28 14:33:14, bshe wrote: > lgtm > > https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... > File chrome/browser/android/vr_shell/ui_interface.cc (right): > > https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... > chrome/browser/android/vr_shell/ui_interface.cc:51: handler_ = handler; > On 2016/10/28 14:16:58, cjgrant wrote: > > On 2016/10/27 20:27:44, bshe wrote: > > > Is the diff suppose to be here? The function was defined in header file but > > not > > > implemented before your patch? > > > > This was intentional. I'm just moving the function to match the (better) > order > > in the header file. > > hah. I missed the deleted function.. > > https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... > File chrome/browser/android/vr_shell/vr_web_contents_observer.cc (right): > > https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... > chrome/browser/android/vr_shell/vr_web_contents_observer.cc:31: > ui_interface_->SetURL(navigation_handle->GetURL()); > On 2016/10/28 14:16:59, cjgrant wrote: > > On 2016/10/27 20:27:44, bshe wrote: > > > Not sure if redirect URL works? > > > > You were right, this was incomplete. I now cover all three navigation > > callbacks. I used a test page with a redirect to verify that we now see the > > following callbacks: > > > > Start: http://wikipedia.com/ > > Redirect: http://www.wikipedia.org/ > > Redirect: https://www.wikipedia.org/ > > Finish: https://www.wikipedia.org/ > > > > Much better. > > > > Do you need to update intermediate urls? It looks like Clank's ominibox only > shows first and last url? Or perhaps it flashes too fast and I can't see. But in > any case, would be nice to keep the behavior the same as Clank, whatever Clank > use. I thought it'd be good to show the intermediate URLs, so that if a page takes a while to load, we know which page it actually is. In terms of mirroring Clank (or desktop Chrome for that matter), there's a lot of work ahead to share code. I don't think we need to track Clank-specific behaviour too closely at this stage. But if you'd like me to remove the redirect updates, that's fine. :)
On 2016/10/28 18:02:51, cjgrant wrote: > On 2016/10/28 14:33:14, bshe wrote: > > lgtm > > > > > https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... > > File chrome/browser/android/vr_shell/ui_interface.cc (right): > > > > > https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... > > chrome/browser/android/vr_shell/ui_interface.cc:51: handler_ = handler; > > On 2016/10/28 14:16:58, cjgrant wrote: > > > On 2016/10/27 20:27:44, bshe wrote: > > > > Is the diff suppose to be here? The function was defined in header file > but > > > not > > > > implemented before your patch? > > > > > > This was intentional. I'm just moving the function to match the (better) > > order > > > in the header file. > > > > hah. I missed the deleted function.. > > > > > https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... > > File chrome/browser/android/vr_shell/vr_web_contents_observer.cc (right): > > > > > https://codereview.chromium.org/2461503002/diff/1/chrome/browser/android/vr_s... > > chrome/browser/android/vr_shell/vr_web_contents_observer.cc:31: > > ui_interface_->SetURL(navigation_handle->GetURL()); > > On 2016/10/28 14:16:59, cjgrant wrote: > > > On 2016/10/27 20:27:44, bshe wrote: > > > > Not sure if redirect URL works? > > > > > > You were right, this was incomplete. I now cover all three navigation > > > callbacks. I used a test page with a redirect to verify that we now see the > > > following callbacks: > > > > > > Start: http://wikipedia.com/ > > > Redirect: http://www.wikipedia.org/ > > > Redirect: https://www.wikipedia.org/ > > > Finish: https://www.wikipedia.org/ > > > > > > Much better. > > > > > > > Do you need to update intermediate urls? It looks like Clank's ominibox only > > shows first and last url? Or perhaps it flashes too fast and I can't see. But > in > > any case, would be nice to keep the behavior the same as Clank, whatever Clank > > use. > > I thought it'd be good to show the intermediate URLs, so that if a page takes a > while to load, we know which page it actually is. > > In terms of mirroring Clank (or desktop Chrome for that matter), there's a lot > of work ahead to share code. I don't think we need to track Clank-specific > behaviour too closely at this stage. But if you'd like me to remove the > redirect updates, that's fine. :) lgtm as it is
The CQ bit was checked by cjgrant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org Link to the patchset: https://codereview.chromium.org/2461503002/#ps20001 (title: "Improvements + nits.")
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) 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 cjgrant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org, bshe@chromium.org Link to the patchset: https://codereview.chromium.org/2461503002/#ps40001 (title: "Rebase")
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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 cjgrant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org, bshe@chromium.org Link to the patchset: https://codereview.chromium.org/2461503002/#ps60001 (title: "Rebase further ahead to resolve conflict.")
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 #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Provide URL-related info to the VR HTML UI. This lets UI render a basic URL bar. BUG=641508 ========== to ========== Provide URL-related info to the VR HTML UI. This lets UI render a basic URL bar. BUG=641508 Committed: https://crrev.com/f0cecd7df4cb20f9c5302d44da3547de4895a521 Cr-Commit-Position: refs/heads/master@{#428621} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f0cecd7df4cb20f9c5302d44da3547de4895a521 Cr-Commit-Position: refs/heads/master@{#428621} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
