Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2410043003:
Add a console messsage for HTTP-bad (Closed)
DescriptionAdd a console messsage for HTTP-bad
This CL logs a console message (at most once per navigation) when
ChromeSecurityStateModelClient::GetSecurityInfo() returns a security
level of HTTP_SHOW_WARNING. This message will allow us to start testing
when password/credit card detection will trigger a "Not secure" warning
in the omnibox, and will allow developers to start testing their sites.
BUG=647561
TEST=In chrome://flags, set #mark-non-secure-as to "Display a verbose
state when password or credit card fields are detected on an HTTP
page". Visit an HTTP page with a password field, like
http://nytimes.com. Open Developer Tools and observe a warning message
about the "Not secure" warning in the console.
Committed: https://crrev.com/f8b269e58af3d7413986fb3196ce9e3f2d67a72f
Cr-Commit-Position: refs/heads/master@{#425739}
Patch Set 1 #
Total comments: 2
Patch Set 2 : elawrence comments #Patch Set 3 : rebase on bug fix https://codereview.chromium.org/2408393003/ #Patch Set 4 : add a browser test #Patch Set 5 : rebase #Patch Set 6 : add test for subframe navigation #
Total comments: 6
Patch Set 7 : rebase #Patch Set 8 : move to Browser::VisibleSSLStateChanged #Patch Set 9 : cleanup #
Total comments: 11
Patch Set 10 : elawrence, sky comments #
Total comments: 9
Patch Set 11 : sky comments #Patch Set 12 : sky comments, take 2 #Patch Set 13 : rebase #Patch Set 14 : de-const other WebContentsDelegates #
Total comments: 3
Messages
Total messages: 65 (36 generated)
The CQ bit was checked by estark@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...
estark@chromium.org changed reviewers: + elawrence@chromium.org, felt@chromium.org
felt, elawrence, PTAL? I'm not thrilled about doing this inside a const getter (GetSecurityInfo). It requires the use of a mutable member to stop from logging multiple times on the same navigation. :/ But I can't think of anywhere better to put it; there's nowhere in //chrome that gets explicitly notified when we set DISPLAYED_PASSWORD_FIELD_ON_HTTP.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks reasonable to me. When you refer to "once per navigation" is that equivalent to "once per top-level navigation, regardless of how many subframes navigate?" If so, I trust that the threading works such that we can't ever have multiple callers invoking MaybeLogHttpWarning in parallel? https://codereview.chromium.org/2410043003/diff/1/chrome/browser/ssl/chrome_s... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2410043003/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client.cc:175: base::StringPrintf("In Chrome M56 (Jan 2017), this page will be marked " I'll betray my lack of understanding of Chrome's use of strings by asking why we need a printf here when it feels like a normal string constant should work? Do we have an official document that defines the public name for UI pieces? E.g. "URL bar" vs. "Omnibox" vs. "Security Chip" etc?
cc emilyschechter: see Eric's question at https://codereview.chromium.org/2410043003/diff/1/chrome/browser/ssl/chrome_s... about the choice of "URL bar" in the devtools string > When you refer to "once per navigation" is that equivalent to "once per > top-level navigation, regardless of how many subframes navigate?" If so, I trust > that the threading works such that we can't ever have multiple callers invoking > MaybeLogHttpWarning in parallel? Good point about the frame navigations; I was missing a check to only reset the flag on main frame navigations. And yeah, this code only runs on the UI thread in the browser, so we can't have multiple MaybeLogHttpWarning calls at once. Finally, I need to figure out how to write a test for this. https://codereview.chromium.org/2410043003/diff/1/chrome/browser/ssl/chrome_s... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2410043003/diff/1/chrome/browser/ssl/chrome_s... chrome/browser/ssl/chrome_security_state_model_client.cc:175: base::StringPrintf("In Chrome M56 (Jan 2017), this page will be marked " On 2016/10/12 02:09:45, elawrence wrote: > I'll betray my lack of understanding of Chrome's use of strings by asking why we > need a printf here when it feels like a normal string constant should work? Err... that would be because I mindlessly based this on a different console message which also uses base::StringPrintf for apparently no reason. Fixed. > > Do we have an official document that defines the public name for UI pieces? E.g. > "URL bar" vs. "Omnibox" vs. "Security Chip" etc? Not sure. I'll cc emilyschechter, got the string from her.
Ha. Good news: I figured out how to write a test for this. Bad news: test caught a bug. So feel free to hold off on further review here until I fix the bug (will do in a separate CL). I'll update this with the test when that's done.
Patchset #5 (id:80001) has been deleted
Patchset #6 (id:120001) has been deleted
The CQ bit was checked by estark@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...
estark@chromium.org changed reviewers: + meacer@chromium.org - felt@chromium.org
Alrighty, this is ready for review again. I rebased on the bug fix (https://codereview.chromium.org/2408393003/, https://codereview.chromium.org/2410023003/) and added browser tests. Kicking over to meacer instead of felt in case felt is busy, but feel free to review if you want to, felt. https://codereview.chromium.org/2410043003/diff/140001/chrome/browser/ssl/chr... File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc (right): https://codereview.chromium.org/2410043003/diff/140001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:1219: // one additional console message. Note: these tests only sort of make sense. We really want to assert that there is no console message after calling GetSecurityInfo() a second time on the same navigation. But the console message notifcation is async, and there's no way to test that a console message never arrives. So this test is not testing precisely what we'd want to test, but I think it's close enough.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/12 22:57:10, estark wrote: > Alrighty, this is ready for review again. I rebased on the bug fix > (https://codereview.chromium.org/2408393003/, > https://codereview.chromium.org/2410023003/) and added browser tests. > > Kicking over to meacer instead of felt in case felt is busy, but feel free to > review if you want to, felt. I can review, but I'll wait until Eric is finished with his review first. Eric: do you want to do another review pass? > > https://codereview.chromium.org/2410043003/diff/140001/chrome/browser/ssl/chr... > File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc > (right): > > https://codereview.chromium.org/2410043003/diff/140001/chrome/browser/ssl/chr... > chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:1219: // > one additional console message. > Note: these tests only sort of make sense. We really want to assert that there > is no console message after calling GetSecurityInfo() a second time on the same > navigation. But the console message notifcation is async, and there's no way to > test that a console message never arrives. So this test is not testing precisely > what we'd want to test, but I think it's close enough.
Description was changed from ========== Add a console messsage for HTTP-bad This CL logs a console message (at most once per navigation) when ChromeSecurityStateModelClient::GetSecurityInfo() returns a security level of HTTP_SHOW_WARNING. This message will allow us to start testing when password/credit card detection will trigger a "Not secure" warning in the omnibox, and will allow developers to start testing their sites. BUG=647561 TEST=In chrome://flags, set #mark-non-secure-as to "Display a verbose state when password or credit card fields are detected on an HTTP page". Visit an HTTP page with a password field, like http://nytimes.com. Open Developer Tools and observe a warning message about the "Not secure" warning in the console. ========== to ========== Add a console messsage for HTTP-bad This CL logs a console message (at most once per navigation) when ChromeSecurityStateModelClient::GetSecurityInfo() returns a security level of HTTP_SHOW_WARNING. This message will allow us to start testing when password/credit card detection will trigger a "Not secure" warning in the omnibox, and will allow developers to start testing their sites. BUG=647561 TEST=In chrome://flags, set #mark-non-secure-as to "Display a verbose state when password or credit card fields are detected on an HTTP page". Visit an HTTP page with a password field, like http://nytimes.com. Open Developer Tools and observe a warning message about the "Not secure" warning in the console. ==========
felt@chromium.org changed reviewers: + felt@chromium.org
I forgot to send my comments (questions, rather) yesterday. https://codereview.chromium.org/2410043003/diff/140001/chrome/browser/ssl/chr... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2410043003/diff/140001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client.cc:326: } I'm sure there is a good reason, but it feels a bit funny to log the message from a getter. Is the idea that we might need to add the message any time page state changes? Can we not observe security state changes instead? https://codereview.chromium.org/2410043003/diff/140001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client.cc:348: } Should we clear this bit when there is a new navigation, instead of the current one finishing? Or is the assumption that we'll never log the message while the navigation is happening, so both events are equal from our perspective?
https://codereview.chromium.org/2410043003/diff/140001/chrome/browser/ssl/chr... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2410043003/diff/140001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client.cc:326: } On 2016/10/13 17:41:01, Mustafa Emre Acer wrote: > I'm sure there is a good reason, but it feels a bit funny to log the message > from a getter. Is the idea that we might need to add the message any time page > state changes? Can we not observe security state changes instead? Yeah, I agree it's kinda gross, see my first comment in the CL thread (that was before I had added you to the review). Looking at it with fresh eyes, though, I think maybe it would be better to do this from within Browser::VisibleSSLStateChanged(). I'll poke around a little bit to make sure that it does what I expect. https://codereview.chromium.org/2410043003/diff/140001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client.cc:348: } On 2016/10/13 17:41:01, Mustafa Emre Acer wrote: > Should we clear this bit when there is a new navigation, instead of the current > one finishing? Or is the assumption that we'll never log the message while the > navigation is happening, so both events are equal from our perspective? (will revisit this comment once I settle the question of whether we can instead do this within Browser::VisibleSSLStateChanged)
The CQ bit was checked by estark@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 checked by estark@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...
estark@chromium.org changed reviewers: + msw@chromium.org
Ok, updated to log the message in Browser::VisibleSSLStateChanged instead in the getter. +msw for chrome/browser/ui, please
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2410043003/diff/200001/chrome/browser/ui/brow... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2410043003/diff/200001/chrome/browser/ui/brow... chrome/browser/ui/browser.cc:1435: if (!logged_http_warning_on_current_navigation_) { I'm not sure if this is the right place for this logging; I'm definitely not the best reviewer for this change; maybe try Scott for this one.
estark@chromium.org changed reviewers: + sky@chromium.org - msw@chromium.org
+sky for chrome/browser/ui, at msw's suggestion
Looks good to me; minor nits and a question about adding a missing test. https://codereview.chromium.org/2410043003/diff/200001/chrome/browser/ssl/chr... File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc (right): https://codereview.chromium.org/2410043003/diff/200001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:1180: // main-frame navigation. Do we need a test for "navigation-like" operations, e.g. window.pushState? https://codereview.chromium.org/2410043003/diff/200001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:1250: // subframe navigations. I think this means "Test that additional HTTP_SHOW_WARNING console messages are not printed after subframe navigations", right? I think we do want a subframe containing a sensitive input to send a console message if there isn't one already present, right? Should we have a test for that? https://codereview.chromium.org/2410043003/diff/200001/chrome/browser/ui/brow... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2410043003/diff/200001/chrome/browser/ui/brow... chrome/browser/ui/browser.cc:1448: "information see https://goo.gl/zmWq3m"); trivial: comma after "information"? https://codereview.chromium.org/2410043003/diff/200001/chrome/browser/ui/brow... File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/2410043003/diff/200001/chrome/browser/ui/brow... chrome/browser/ui/browser.h:1008: // True if a console has been logged about an upcoming omnibox warning The word "upcoming" feels ambiguous? Does this mean "The console warning immediately precedes the state change in the security chip"? Or does it mean "A future version of Chrome will warn", or ?
https://codereview.chromium.org/2410043003/diff/200001/chrome/browser/ui/brow... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2410043003/diff/200001/chrome/browser/ui/brow... chrome/browser/ui/browser.cc:1665: logged_http_warning_on_current_navigation_ = false; Browser is the delegate of multiple WebContents. logged_http_warning_on_current_navigation_ is really specific to a WebContents. So, having Browser track per WebContents state doesn't make sense and won't really work. ChromeSecurityStateModelClient is per WebContents, and seems to be the place that is tracking related information. Would it make sense to have these two functions call equivalent functions on ChromeSecurityStateModelClient, moving logged_http_warning_on_current_navigation_ there? By doing that logged_http_warning_on_current_navigation_ is scoped to a WebContents, which is what you want.
sky: I moved the logging from Browser to a ChromeSSMClient method called by Browser::VisibleSSLStateChanged. https://codereview.chromium.org/2410043003/diff/140001/chrome/browser/ssl/chr... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2410043003/diff/140001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client.cc:348: } On 2016/10/13 17:59:17, estark wrote: > On 2016/10/13 17:41:01, Mustafa Emre Acer wrote: > > Should we clear this bit when there is a new navigation, instead of the > current > > one finishing? Or is the assumption that we'll never log the message while the > > navigation is happening, so both events are equal from our perspective? > > (will revisit this comment once I settle the question of whether we can instead > do this within Browser::VisibleSSLStateChanged) I think it's more correct to clear the flag when the navigation finishes. If the navigation stops, for example, and the pending entry is thrown out, then we don't want to have reset the flag. Does that make sense? https://codereview.chromium.org/2410043003/diff/200001/chrome/browser/ssl/chr... File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc (right): https://codereview.chromium.org/2410043003/diff/200001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:1180: // main-frame navigation. On 2016/10/14 15:12:27, elawrence wrote: > Do we need a test for "navigation-like" operations, e.g. window.pushState? Done. I think (?) we can distinguish with NavigationHandle::IsSynchronousNavigation() https://codereview.chromium.org/2410043003/diff/200001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:1250: // subframe navigations. On 2016/10/14 15:12:27, elawrence wrote: > I think this means "Test that additional HTTP_SHOW_WARNING console messages are > not printed after subframe navigations", right? I think we do want a subframe > containing a sensitive input to send a console message if there isn't one > already present, right? Should we have a test for that? I clarified the comment, but I don't think it's necessary to add a test for it, because we already have tests elsewhere that a sensitive input in a subframe triggers HTTP_SHOW_WARNING. The point of these tests is to test that the console message gets printed on the HTTP_SHOW_WARNING state, and that the navigation-reset logic is working properly; I don't think we need tests for every condition that can trigger HTTP_SHOW_WARNING. https://codereview.chromium.org/2410043003/diff/200001/chrome/browser/ui/brow... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2410043003/diff/200001/chrome/browser/ui/brow... chrome/browser/ui/browser.cc:1448: "information see https://goo.gl/zmWq3m"); On 2016/10/14 15:12:27, elawrence wrote: > trivial: comma after "information"? Done. https://codereview.chromium.org/2410043003/diff/200001/chrome/browser/ui/brow... chrome/browser/ui/browser.cc:1665: logged_http_warning_on_current_navigation_ = false; On 2016/10/14 16:44:03, sky wrote: > Browser is the delegate of multiple WebContents. > logged_http_warning_on_current_navigation_ is really specific to a WebContents. > So, having Browser track per WebContents state doesn't make sense and won't > really work. > > ChromeSecurityStateModelClient is per WebContents, and seems to be the place > that is tracking related information. Would it make sense to have these two > functions call equivalent functions on ChromeSecurityStateModelClient, moving > logged_http_warning_on_current_navigation_ there? By doing that > logged_http_warning_on_current_navigation_ is scoped to a WebContents, which is > what you want. Ah, right, that makes sense. I forwarded VisibleSSLStateChanged to the ChromeSSMClient. And I just made ChromeSSMClient a WebContentsObserver so that it can observe navigations itself. (I already in fact already done that in an earlier patchset.) Please take another look. https://codereview.chromium.org/2410043003/diff/200001/chrome/browser/ui/brow... File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/2410043003/diff/200001/chrome/browser/ui/brow... chrome/browser/ui/browser.h:1008: // True if a console has been logged about an upcoming omnibox warning On 2016/10/14 15:12:28, elawrence wrote: > The word "upcoming" feels ambiguous? Does this mean "The console warning > immediately precedes the state change in the security chip"? Or does it mean "A > future version of Chrome will warn", or ? Done.
https://codereview.chromium.org/2410043003/diff/220001/chrome/browser/ssl/chr... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2410043003/diff/220001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client.cc:307: if (!logged_http_warning_on_current_navigation_) { optional: early out (we generally do that rather than have a big indented block like you have here). https://codereview.chromium.org/2410043003/diff/220001/chrome/browser/ssl/chr... File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc (right): https://codereview.chromium.org/2410043003/diff/220001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:1251: IN_PROC_BROWSER_TEST_F(ChromeSecurityStateModelClientTestWithPasswordCcSwitch, Wow, this sure is a lot of test code for a simple change! Can these be made unit tests? Specifically using WebContentsTester? https://codereview.chromium.org/2410043003/diff/220001/chrome/browser/ui/brow... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2410043003/diff/220001/chrome/browser/ui/brow... chrome/browser/ui/browser.cc:1432: if (active_contents == source) { Why do you only care about the active webcontents? What happens if a navigation is started in the background that matches your criteria and then the user switches to the webcontents. Shouldn't they see the message?
The CQ bit was checked by estark@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 checked by estark@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/2410043003/diff/220001/chrome/browser/ssl/chr... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2410043003/diff/220001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client.cc:307: if (!logged_http_warning_on_current_navigation_) { On 2016/10/14 22:21:47, sky wrote: > optional: early out (we generally do that rather than have a big indented block > like you have here). Done. https://codereview.chromium.org/2410043003/diff/220001/chrome/browser/ssl/chr... File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc (right): https://codereview.chromium.org/2410043003/diff/220001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:1251: IN_PROC_BROWSER_TEST_F(ChromeSecurityStateModelClientTestWithPasswordCcSwitch, On 2016/10/14 22:21:47, sky wrote: > Wow, this sure is a lot of test code for a simple change! > > Can these be made unit tests? Specifically using WebContentsTester? I couldn't figure out how to do them as unit tests because the WebContentsDelegate::AddMessageToConsole() method that I'm mocking out gets called on a signal from Blink. That is, when the browser process logs a message to the console, it gets sent down to the renderer which prints it and then signals back up to the browser that a message has been logged, which is what I'm mocking to check that the message was actually logged. I couldn't find a way to test that a console message had been logged from a unit test. (MockRenderProcessHost::sink() isn't usable for this because the console message IPC is not defined as part of the public content API.) https://codereview.chromium.org/2410043003/diff/220001/chrome/browser/ui/brow... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2410043003/diff/220001/chrome/browser/ui/brow... chrome/browser/ui/browser.cc:1432: if (active_contents == source) { On 2016/10/14 22:21:47, sky wrote: > Why do you only care about the active webcontents? What happens if a navigation > is started in the background that matches your criteria and then the user > switches to the webcontents. Shouldn't they see the message? Done, but there's an unfortunate side effect of this that I have to non-const |source|. I think that's okay, though, because content interfaces are explicitly not supposed to have const identifiers (https://dev.chromium.org/developers/content-module/content-api).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by estark@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.
https://codereview.chromium.org/2410043003/diff/220001/chrome/browser/ui/brow... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2410043003/diff/220001/chrome/browser/ui/brow... chrome/browser/ui/browser.cc:1432: if (active_contents == source) { On 2016/10/14 23:50:01, estark wrote: > On 2016/10/14 22:21:47, sky wrote: > > Why do you only care about the active webcontents? What happens if a > navigation > > is started in the background that matches your criteria and then the user > > switches to the webcontents. Shouldn't they see the message? > > Done, but there's an unfortunate side effect of this that I have to non-const > |source|. I think that's okay, though, because content interfaces are explicitly > not supposed to have const identifiers > (https://dev.chromium.org/developers/content-module/content-api). Ick. I think it better to change the function to not take a const (line 1431 effectively gets around the const too).
https://codereview.chromium.org/2410043003/diff/220001/chrome/browser/ui/brow... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2410043003/diff/220001/chrome/browser/ui/brow... chrome/browser/ui/browser.cc:1432: if (active_contents == source) { On 2016/10/17 15:41:48, sky wrote: > On 2016/10/14 23:50:01, estark wrote: > > On 2016/10/14 22:21:47, sky wrote: > > > Why do you only care about the active webcontents? What happens if a > > navigation > > > is started in the background that matches your criteria and then the user > > > switches to the webcontents. Shouldn't they see the message? > > > > Done, but there's an unfortunate side effect of this that I have to non-const > > |source|. I think that's okay, though, because content interfaces are > explicitly > > not supposed to have const identifiers > > (https://dev.chromium.org/developers/content-module/content-api). > > Ick. I think it better to change the function to not take a const (line 1431 > effectively gets around the const too). Why? This API explicitly goes against the content API design guidelines, why not change it to match? (I don't think line 1431 has anything to do with getting around the const; it just serves to avoid unnecessary toolbar updates. In fact AFAICT GetActiveWebContents() ends up flowing to a const parameter in LocationBarView::Update().)
https://codereview.chromium.org/2410043003/diff/220001/chrome/browser/ui/brow... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2410043003/diff/220001/chrome/browser/ui/brow... chrome/browser/ui/browser.cc:1432: if (active_contents == source) { On 2016/10/17 16:11:13, estark wrote: > On 2016/10/17 15:41:48, sky wrote: > > On 2016/10/14 23:50:01, estark wrote: > > > On 2016/10/14 22:21:47, sky wrote: > > > > Why do you only care about the active webcontents? What happens if a > > > navigation > > > > is started in the background that matches your criteria and then the user > > > > switches to the webcontents. Shouldn't they see the message? > > > > > > Done, but there's an unfortunate side effect of this that I have to > non-const > > > |source|. I think that's okay, though, because content interfaces are > > explicitly > > > not supposed to have const identifiers > > > (https://dev.chromium.org/developers/content-module/content-api). > > > > Ick. I think it better to change the function to not take a const (line 1431 > > effectively gets around the const too). > > Why? This API explicitly goes against the content API design guidelines, why not > change it to match? > > (I don't think line 1431 has anything to do with getting around the const; it > just serves to avoid unnecessary toolbar updates. In fact AFAICT > GetActiveWebContents() ends up flowing to a const parameter in > LocationBarView::Update().) Actually re-reading your comment I'm not sure I understood what you're suggesting: what do you mean by "change the function to not take a const"? (what function?)
I was a bit confused by your early comment. I thought you were adding a const_cast, which is bad. Updating the function to take a non-const is the right thing, which you did. LGTM
https://codereview.chromium.org/2410043003/diff/300001/chrome/browser/ui/brow... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2410043003/diff/300001/chrome/browser/ui/brow... chrome/browser/ui/browser.cc:1435: ChromeSecurityStateModelClient::FromWebContents(source); To make sure I understand: (1) Something triggers DidChangeVisibleSSLState (like a load is committed, or mixed content detected) which leads to this (2) UpdateToolbar will, down the line, cause CSSMC to be updated (3) This will then tell the CSSMC to maybe display the console message This presumably assumes, then, that #2 has happened before #3 -- that UpdateToolbar does cause the SSM to update. Or else it won't know that it needs to display the console message. Yeah? Which is sort of weird.
https://codereview.chromium.org/2410043003/diff/300001/chrome/browser/ui/brow... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2410043003/diff/300001/chrome/browser/ui/brow... chrome/browser/ui/browser.cc:1435: ChromeSecurityStateModelClient::FromWebContents(source); On 2016/10/17 16:59:33, felt wrote: > To make sure I understand: > > (1) Something triggers DidChangeVisibleSSLState (like a load is committed, or > mixed content detected) which leads to this > > (2) UpdateToolbar will, down the line, cause CSSMC to be updated > > (3) This will then tell the CSSMC to maybe display the console message > > This presumably assumes, then, that #2 has happened before #3 -- that > UpdateToolbar does cause the SSM to update. Or else it won't know that it needs > to display the console message. Yeah? Which is sort of weird. CSSMC::VisibleSSLStateChanged() computes the SecurityLevel fresh from the current SSLStatus to check whether it needs to display the console message. So it doesn't rely on #2 having happened first. (It is a tad wasteful, but it should only happen once on navigation and when there's a change like mixed content, which is relatively rarely compared to how often the omnibox redrawing causes the security level to be recomputed.)
lgtm https://codereview.chromium.org/2410043003/diff/300001/chrome/browser/ui/brow... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2410043003/diff/300001/chrome/browser/ui/brow... chrome/browser/ui/browser.cc:1435: ChromeSecurityStateModelClient::FromWebContents(source); On 2016/10/17 17:14:20, estark wrote: > On 2016/10/17 16:59:33, felt wrote: > > To make sure I understand: > > > > (1) Something triggers DidChangeVisibleSSLState (like a load is committed, or > > mixed content detected) which leads to this > > > > (2) UpdateToolbar will, down the line, cause CSSMC to be updated > > > > (3) This will then tell the CSSMC to maybe display the console message > > > > This presumably assumes, then, that #2 has happened before #3 -- that > > UpdateToolbar does cause the SSM to update. Or else it won't know that it > needs > > to display the console message. Yeah? Which is sort of weird. > > CSSMC::VisibleSSLStateChanged() computes the SecurityLevel fresh from the > current SSLStatus to check whether it needs to display the console message. So > it doesn't rely on #2 having happened first. > > (It is a tad wasteful, but it should only happen once on navigation and when > there's a change like mixed content, which is relatively rarely compared to how > often the omnibox redrawing causes the security level to be recomputed.) Oh I see, I missed that there was a call to GetSecurityInfo at the top. So this just adds one more time it's recomputed. ;) Cool.
estark@chromium.org changed reviewers: + avi@chromium.org, boliu@chromium.org
avi: can you please review the change to content/public? (Removing `const` from an argument per https://dev.chromium.org/developers/content-module/content-api) boliu: can you please review components/web_contents_delegate_android?
On 2016/10/17 17:22:52, estark wrote: > avi: can you please review the change to content/public? (Removing `const` from > an argument per https://dev.chromium.org/developers/content-module/content-api) > > boliu: can you please review components/web_contents_delegate_android? rs lgtm
LGTM
The CQ bit was checked by estark@chromium.org
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.
Description was changed from ========== Add a console messsage for HTTP-bad This CL logs a console message (at most once per navigation) when ChromeSecurityStateModelClient::GetSecurityInfo() returns a security level of HTTP_SHOW_WARNING. This message will allow us to start testing when password/credit card detection will trigger a "Not secure" warning in the omnibox, and will allow developers to start testing their sites. BUG=647561 TEST=In chrome://flags, set #mark-non-secure-as to "Display a verbose state when password or credit card fields are detected on an HTTP page". Visit an HTTP page with a password field, like http://nytimes.com. Open Developer Tools and observe a warning message about the "Not secure" warning in the console. ========== to ========== Add a console messsage for HTTP-bad This CL logs a console message (at most once per navigation) when ChromeSecurityStateModelClient::GetSecurityInfo() returns a security level of HTTP_SHOW_WARNING. This message will allow us to start testing when password/credit card detection will trigger a "Not secure" warning in the omnibox, and will allow developers to start testing their sites. BUG=647561 TEST=In chrome://flags, set #mark-non-secure-as to "Display a verbose state when password or credit card fields are detected on an HTTP page". Visit an HTTP page with a password field, like http://nytimes.com. Open Developer Tools and observe a warning message about the "Not secure" warning in the console. ==========
Message was sent while issue was closed.
Committed patchset #14 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Add a console messsage for HTTP-bad This CL logs a console message (at most once per navigation) when ChromeSecurityStateModelClient::GetSecurityInfo() returns a security level of HTTP_SHOW_WARNING. This message will allow us to start testing when password/credit card detection will trigger a "Not secure" warning in the omnibox, and will allow developers to start testing their sites. BUG=647561 TEST=In chrome://flags, set #mark-non-secure-as to "Display a verbose state when password or credit card fields are detected on an HTTP page". Visit an HTTP page with a password field, like http://nytimes.com. Open Developer Tools and observe a warning message about the "Not secure" warning in the console. ========== to ========== Add a console messsage for HTTP-bad This CL logs a console message (at most once per navigation) when ChromeSecurityStateModelClient::GetSecurityInfo() returns a security level of HTTP_SHOW_WARNING. This message will allow us to start testing when password/credit card detection will trigger a "Not secure" warning in the omnibox, and will allow developers to start testing their sites. BUG=647561 TEST=In chrome://flags, set #mark-non-secure-as to "Display a verbose state when password or credit card fields are detected on an HTTP page". Visit an HTTP page with a password field, like http://nytimes.com. Open Developer Tools and observe a warning message about the "Not secure" warning in the console. Committed: https://crrev.com/f8b269e58af3d7413986fb3196ce9e3f2d67a72f Cr-Commit-Position: refs/heads/master@{#425739} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/f8b269e58af3d7413986fb3196ce9e3f2d67a72f Cr-Commit-Position: refs/heads/master@{#425739} |
