Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1028503002:
Don't show location bar in bookmark apps if there has been no navigation (Closed)
|
Created:
5 years, 9 months ago by benwells Modified:
5 years, 9 months ago Reviewers:
felt CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't show location bar in bookmark apps if there has been no navigation
This prevents the location bar appearing and then disappearing when a
bookmark app is first opened.
BUG=463405
Committed: https://crrev.com/b57d4b15bf347d7a65282abd81342dc176a760ec
Cr-Commit-Position: refs/heads/master@{#321964}
Patch Set 1 #
Total comments: 3
Messages
Total messages: 12 (2 generated)
benwells@chromium.org changed reviewers: + felt@chromium.org
https://codereview.chromium.org/1028503002/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_ui_util.cc (right): https://codereview.chromium.org/1028503002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_ui_util.cc:83: if (web_contents->GetLastCommittedURL().is_empty()) Can you test what happens here if you navigate to a page that shows a warning?
https://codereview.chromium.org/1028503002/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_ui_util.cc (right): https://codereview.chromium.org/1028503002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_ui_util.cc:83: if (web_contents->GetLastCommittedURL().is_empty()) On 2015/03/23 20:41:56, felt wrote: > Can you test what happens here if you navigate to a page that shows a warning? I've put a screenshot on the bug. Let me know if that is what you meant.
lgtm https://codereview.chromium.org/1028503002/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_ui_util.cc (right): https://codereview.chromium.org/1028503002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_ui_util.cc:83: if (web_contents->GetLastCommittedURL().is_empty()) On 2015/03/24 05:14:59, benwells wrote: > On 2015/03/23 20:41:56, felt wrote: > > Can you test what happens here if you navigate to a page that shows a warning? > > I've put a screenshot on the bug. Let me know if that is what you meant. Thanks, yes, that's what I meant. I wanted to check that the committed entry wouldn't end up empty if you hit an interstitial on the first navigation, or something weird like that.
On 2015/03/24 05:27:25, felt wrote: > lgtm > > https://codereview.chromium.org/1028503002/diff/1/chrome/browser/extensions/e... > File chrome/browser/extensions/extension_ui_util.cc (right): > > https://codereview.chromium.org/1028503002/diff/1/chrome/browser/extensions/e... > chrome/browser/extensions/extension_ui_util.cc:83: if > (web_contents->GetLastCommittedURL().is_empty()) > On 2015/03/24 05:14:59, benwells wrote: > > On 2015/03/23 20:41:56, felt wrote: > > > Can you test what happens here if you navigate to a page that shows a > warning? > > > > I've put a screenshot on the bug. Let me know if that is what you meant. > > Thanks, yes, that's what I meant. I wanted to check that the committed entry > wouldn't end up empty if you hit an interstitial on the first navigation, or > something weird like that. OK. Maybe I didn't test what you wanted exactly. What I showed you was what happens if someone has a bookmark app to www.goodurl.com and navigates to https://badcert.com. If the bookmark app is itself pointed at https://badcert.com, it actually skips the warning and doesn't show the URL bar. That is probably bad and I only found it out then, but is kind of orthogonal to this (i.e. it will happen with or without this change). I've logged crbug.com/469993 for this. So I think this still should lg but maybe you want to confirm.
On 2015/03/24 05:39:45, benwells wrote: > On 2015/03/24 05:27:25, felt wrote: > > lgtm > > > > > https://codereview.chromium.org/1028503002/diff/1/chrome/browser/extensions/e... > > File chrome/browser/extensions/extension_ui_util.cc (right): > > > > > https://codereview.chromium.org/1028503002/diff/1/chrome/browser/extensions/e... > > chrome/browser/extensions/extension_ui_util.cc:83: if > > (web_contents->GetLastCommittedURL().is_empty()) > > On 2015/03/24 05:14:59, benwells wrote: > > > On 2015/03/23 20:41:56, felt wrote: > > > > Can you test what happens here if you navigate to a page that shows a > > warning? > > > > > > I've put a screenshot on the bug. Let me know if that is what you meant. > > > > Thanks, yes, that's what I meant. I wanted to check that the committed entry > > wouldn't end up empty if you hit an interstitial on the first navigation, or > > something weird like that. > > OK. Maybe I didn't test what you wanted exactly. What I showed you was what > happens if someone has a bookmark app to http://www.goodurl.com and navigates to > https://badcert.com. > > If the bookmark app is itself pointed at https://badcert.com, it actually skips > the warning and doesn't show the URL bar. That is probably bad and I only found > it out then, but is kind of orthogonal to this (i.e. it will happen with or > without this change). I've logged crbug.com/469993 for this. > > So I think this still should lg but maybe you want to confirm. still lgtm, I was actually asking about the first case but glad you tested the second as well so that we now know about that bug :)
On 2015/03/24 05:42:26, felt wrote: > On 2015/03/24 05:39:45, benwells wrote: > > On 2015/03/24 05:27:25, felt wrote: > > > lgtm > > > > > > > > > https://codereview.chromium.org/1028503002/diff/1/chrome/browser/extensions/e... > > > File chrome/browser/extensions/extension_ui_util.cc (right): > > > > > > > > > https://codereview.chromium.org/1028503002/diff/1/chrome/browser/extensions/e... > > > chrome/browser/extensions/extension_ui_util.cc:83: if > > > (web_contents->GetLastCommittedURL().is_empty()) > > > On 2015/03/24 05:14:59, benwells wrote: > > > > On 2015/03/23 20:41:56, felt wrote: > > > > > Can you test what happens here if you navigate to a page that shows a > > > warning? > > > > > > > > I've put a screenshot on the bug. Let me know if that is what you meant. > > > > > > Thanks, yes, that's what I meant. I wanted to check that the committed entry > > > wouldn't end up empty if you hit an interstitial on the first navigation, or > > > something weird like that. > > > > OK. Maybe I didn't test what you wanted exactly. What I showed you was what > > happens if someone has a bookmark app to http://www.goodurl.com and navigates > to > > https://badcert.com. > > > > If the bookmark app is itself pointed at https://badcert.com, it actually > skips > > the warning and doesn't show the URL bar. That is probably bad and I only > found > > it out then, but is kind of orthogonal to this (i.e. it will happen with or > > without this change). I've logged crbug.com/469993 for this. > > > > So I think this still should lg but maybe you want to confirm. > > still lgtm, I was actually asking about the first case but glad you tested the > second as well so that we now know about that bug :) cool bananas....
The CQ bit was checked by benwells@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1028503002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/b57d4b15bf347d7a65282abd81342dc176a760ec Cr-Commit-Position: refs/heads/master@{#321964} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
