Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2899393003:
[vr] Exit VR mode when we encounter unsupported URLs (Closed)
|
Created:
3 years, 6 months ago by Ian Vollick Modified:
3 years, 6 months ago CC:
chromium-reviews, feature-vr-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[vr] Exit VR mode when we encounter unsupported URLs
We currently do not handle URL elision, so we will
exit VR mode.
BUG=727373
Review-Url: https://codereview.chromium.org/2899393003
Cr-Commit-Position: refs/heads/master@{#475501}
Committed: https://chromium.googlesource.com/chromium/src/+/4b858437b2dc6a958d11aaf01eb1251d303b57b1
Patch Set 1 #Patch Set 2 : . #
Total comments: 1
Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : . #Patch Set 6 : . #Patch Set 7 : . #
Messages
Total messages: 31 (21 generated)
The CQ bit was checked by vollick@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...
Description was changed from ========== [vr] exit VR mode when we encounter unsupported URLs We currently do not handle URL elision, so we will bail. BUG=722861 ========== to ========== [vr] exit VR mode when we encounter unsupported URLs We currently do not handle URL elision, so we will bail. BUG=722861 ==========
vollick@chromium.org changed reviewers: + cjgrant@chromium.org, mthiesse@chromium.org
Description was changed from ========== [vr] exit VR mode when we encounter unsupported URLs We currently do not handle URL elision, so we will bail. BUG=722861 ========== to ========== [vr] Exit VR mode when we encounter unsupported URLs We currently do not handle URL elision, so we will exit VR mode. BUG=722861 ==========
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
How temporary is this patch? How difficult is elision? Our current max URL length doesn't seem very long, aren't users going to hit this like all the time? If elision is too hard, could we do something like expanding the URL bar (up to some limit), to mitigate how much this sucks?
On 2017/05/27 04:58:12, mthiesse wrote: > How temporary is this patch? How difficult is elision? > It doesn't currently work well with the emphasis code. It's probably worth connecting offline to talk about the situation in more detail. > Our current max URL length doesn't seem very long, aren't users going to hit > this like all the time? We don't bail whenever the URL is longer than the URL bar, only when elision is required. It wasn't as easy to hit this as you might think. That said, expanding the URL bar seems like a good idea. We'll still eventually hit a limit with that, too, so we'll still need to handle this case, but I think that the expansion would be a good, complementary mitigation. > > If elision is too hard, could we do something like expanding the URL bar (up to > some limit), to mitigate how much this sucks? I'll try to put a CL together for this soon.
lgtm https://codereview.chromium.org/2899393003/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right): https://codereview.chromium.org/2899393003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/textures/url_bar_texture.cc:270: render_text->SetText(text); Just checking that you still want to proceed and render the long URL, rather than leaving the field blank. I think this is the right thing to do.
Patchset #3 (id:40001) has been deleted
On 2017/05/29 15:17:14, cjgrant wrote: > lgtm > > https://codereview.chromium.org/2899393003/diff/20001/chrome/browser/android/... > File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right): > > https://codereview.chromium.org/2899393003/diff/20001/chrome/browser/android/... > chrome/browser/android/vr_shell/textures/url_bar_texture.cc:270: > render_text->SetText(text); > Just checking that you still want to proceed and render the long URL, rather > than leaving the field blank. I think this is the right thing to do. Reminder that we talked about attaching this CL to a new bug, as the existing bug has already been deemed safe to merge to M60. Not sure what happens if we add to it.
Description was changed from ========== [vr] Exit VR mode when we encounter unsupported URLs We currently do not handle URL elision, so we will exit VR mode. BUG=722861 ========== to ========== [vr] Exit VR mode when we encounter unsupported URLs We currently do not handle URL elision, so we will exit VR mode. BUG=727373 ==========
On 2017/05/29 17:57:25, cjgrant wrote: > On 2017/05/29 15:17:14, cjgrant wrote: > > lgtm > > > > > https://codereview.chromium.org/2899393003/diff/20001/chrome/browser/android/... > > File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (right): > > > > > https://codereview.chromium.org/2899393003/diff/20001/chrome/browser/android/... > > chrome/browser/android/vr_shell/textures/url_bar_texture.cc:270: > > render_text->SetText(text); > > Just checking that you still want to proceed and render the long URL, rather > > than leaving the field blank. I think this is the right thing to do. > > Reminder that we talked about attaching this CL to a new bug, as the existing > bug has already been deemed safe to merge to M60. Not sure what happens if we > add to it. Yep, updated.
Patchset #7 (id:140001) has been deleted
The CQ bit was checked by vollick@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.
vollick@chromium.org changed reviewers: + isherman@chromium.org
+isherman for enums.xml
The CQ bit was checked by vollick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cjgrant@chromium.org Link to the patchset: https://codereview.chromium.org/2899393003/#ps160001 (title: ".")
The CQ bit was unchecked by vollick@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by vollick@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": 160001, "attempt_start_ts": 1496147509362020,
"parent_rev": "eb0f08a8b0768450acac29a5230cc2202fc89077", "commit_rev":
"4b858437b2dc6a958d11aaf01eb1251d303b57b1"}
Message was sent while issue was closed.
Description was changed from ========== [vr] Exit VR mode when we encounter unsupported URLs We currently do not handle URL elision, so we will exit VR mode. BUG=727373 ========== to ========== [vr] Exit VR mode when we encounter unsupported URLs We currently do not handle URL elision, so we will exit VR mode. BUG=727373 Review-Url: https://codereview.chromium.org/2899393003 Cr-Commit-Position: refs/heads/master@{#475501} Committed: https://chromium.googlesource.com/chromium/src/+/4b858437b2dc6a958d11aaf01eb1... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001) as https://chromium.googlesource.com/chromium/src/+/4b858437b2dc6a958d11aaf01eb1... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
