Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1189413003:
Added CaptivePortal Interstitial to chrome://interstitials (Closed)
DescriptionAdded CaptivePortal Interstitial to chrome://interstitials
BUG=501961
Committed: https://crrev.com/0c34909aa92c44020f8fdc4a4a1676b6d5d8fcd0
Cr-Commit-Position: refs/heads/master@{#338409}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Accept url params, display different interstitials for wifi and non-wifi networks #
Total comments: 15
Patch Set 3 : Some style changes #
Total comments: 6
Patch Set 4 : Some style changes #
Total comments: 5
Patch Set 5 : Changes to the chrome://interstitials html page #
Total comments: 2
Patch Set 6 : Changes to the chrome://interstitials html page #Patch Set 7 : Changes to the chrome://interstitials html page #
Total comments: 2
Patch Set 8 : Changes to the chrome://interstitials html page #
Total comments: 2
Patch Set 9 : Html Style Changes #Patch Set 10 : html style changes, some other #Patch Set 11 : html style changes #Patch Set 12 : html style changes #
Total comments: 2
Patch Set 13 : html style changes #Patch Set 14 : fixing crash for android builds #Patch Set 15 : fixing crash for android builds-2 #
Messages
Total messages: 43 (10 generated)
bhanudev@google.com changed reviewers: + meacer@chromium.org
https://codereview.chromium.org/1189413003/diff/1/chrome/browser/ui/webui/int... File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): https://codereview.chromium.org/1189413003/diff/1/chrome/browser/ui/webui/int... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:147: GURL landing_url("https://example.com"); You might want to make this a different URL than request_url, since it's the URL the captive portal is redirecting the user to. In fact, how about making it a request param as in CreateSafeBrowsingBlockingPage? So a url like chrome://interstitials/?url=https://google.com&landing_page=https://captive.portal/login&wifi_name=CoffeeShopWifi would display a warning for https://google.com URL, https://captive.portal landing page and CoffeeShopWifi wifi name. https://codereview.chromium.org/1189413003/diff/1/chrome/browser/ui/webui/int... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:152: web_contents, request_url, landing_url,nullptr, ssl_info, Space between landing_url and nullptr. Doing |git cl format| before uploading the code saves some time :)
https://codereview.chromium.org/1189413003/diff/1/chrome/browser/ui/webui/int... File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): https://codereview.chromium.org/1189413003/diff/1/chrome/browser/ui/webui/int... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:147: GURL landing_url("https://example.com"); On 2015/06/19 17:46:25, Mustafa Emre Acer wrote: > You might want to make this a different URL than request_url, since it's the URL > the captive portal is redirecting the user to. > > In fact, how about making it a request param as in > CreateSafeBrowsingBlockingPage? So a url like > > chrome://interstitials/?url=https://google.com&landing_page=https://captive.portal/login&wifi_name=CoffeeShopWifi > > would display a warning for https://google.com URL, https://captive.portal > landing page and CoffeeShopWifi wifi name. I think using request params is a good idea. Thanks. It gives much clear description of page. I will use google.com for request url. For landing page, Can I use an external url like https://captive.portal.com? Or shall I use https://captiveportal.example.com instead because example.com is not owned by anyone. https://codereview.chromium.org/1189413003/diff/1/chrome/browser/ui/webui/int... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:152: web_contents, request_url, landing_url,nullptr, ssl_info, On 2015/06/19 17:46:25, Mustafa Emre Acer wrote: > Space between landing_url and nullptr. > > Doing |git cl format| before uploading the code saves some time :) Thanks. I forgot the command. It really save time.
https://codereview.chromium.org/1189413003/diff/1/chrome/browser/ui/webui/int... File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): https://codereview.chromium.org/1189413003/diff/1/chrome/browser/ui/webui/int... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:147: GURL landing_url("https://example.com"); On 2015/06/19 18:05:41, bhanudev wrote: > On 2015/06/19 17:46:25, Mustafa Emre Acer wrote: > > You might want to make this a different URL than request_url, since it's the > URL > > the captive portal is redirecting the user to. > > > > In fact, how about making it a request param as in > > CreateSafeBrowsingBlockingPage? So a url like > > > > > chrome://interstitials/?url=https://google.com&landing_page=https://captive.portal/login&wifi_name=CoffeeShopWifi > > > > would display a warning for https://google.com URL, https://captive.portal > > landing page and CoffeeShopWifi wifi name. > > I think using request params is a good idea. Thanks. > It gives much clear description of page. > > I will use http://google.com for request url. For landing page, Can I use an external > url like https://captive.portal.com Or shall I use > https://captiveportal.example.com instead because http://example.com is not owned by > anyone. Either one works since we aren't issuing any requests. https://captive.portal looked cleaner to me :)
https://codereview.chromium.org/1189413003/diff/20001/chrome/browser/resource... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/1189413003/diff/20001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:23: <a href='captiveportal?url=https://google.com&landing_page=https://captive.porta... (On Wifi "CoffeeShopWifi")</a><br> Are the labels good, or should I change them. Thanks. https://codereview.chromium.org/1189413003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): https://codereview.chromium.org/1189413003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:65: }; I am not sure if this is the correct place to place the definition of a class, since this is only associated with the CreateCaptivePortalBlockingPage function and not others. https://codereview.chromium.org/1189413003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:197: blocking_page->SetDelegateForTesting(delegate); This is a testing function. I am not sure if can I use it here.
https://codereview.chromium.org/1189413003/diff/20001/chrome/browser/resource... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/1189413003/diff/20001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:23: <a href='captiveportal?url=https://google.com&landing_page=https://captive.porta... (On Wifi "CoffeeShopWifi")</a><br> On 2015/06/19 21:09:03, bhanudev wrote: > Are the labels good, or should I change them. Thanks. How about making this a bit fancier? Add a default captive portal link, and then add variations of the parameters: [Captive Portal] [Wifi] [Wifi (with non-default wifi name)] [Non-Wifi] https://codereview.chromium.org/1189413003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): https://codereview.chromium.org/1189413003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:61: const bool is_wifi_connection_; no need for const for bool. https://codereview.chromium.org/1189413003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:62: const std::string wifi_ssid_; Use a reference here: |const std::string& wifi_ssid_| https://codereview.chromium.org/1189413003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:65: }; On 2015/06/19 21:09:03, bhanudev wrote: > I am not sure if this is the correct place to place the definition of a class, > since this is only associated with the CreateCaptivePortalBlockingPage function > and not others. Looks okay, it's obvious from the base class name where this will be used. https://codereview.chromium.org/1189413003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:162: bool is_wifi_connection = 0; false instead of 0. https://codereview.chromium.org/1189413003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:165: std::string wifi_ssid; If no "wifi_name" query param is passed this will be empty (while your html file doesn't do that, it's possible someone manually enters a URL such as chrome://interstitials/captiveportal?is_wifi=1). You should assign a default value to this. https://codereview.chromium.org/1189413003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:197: blocking_page->SetDelegateForTesting(delegate); On 2015/06/19 21:09:03, bhanudev wrote: > This is a testing function. I am not sure if can I use it here. Oh, right :( Let's make it a non-test method and add a comment saying it should only be used in tests and chrome://interstitials page. That will require you to change a couple of test files.
The interstitial does not redirect to page that login page, if you click the "Connect" button. Is this the expected behavior.. Thanks. https://codereview.chromium.org/1189413003/diff/20001/chrome/browser/resource... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/1189413003/diff/20001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:23: <a href='captiveportal?url=https://google.com&landing_page=https://captive.porta... (On Wifi "CoffeeShopWifi")</a><br> On 2015/06/19 21:23:18, Mustafa Emre Acer wrote: > On 2015/06/19 21:09:03, bhanudev wrote: > > Are the labels good, or should I change them. Thanks. > > How about making this a bit fancier? Add a default captive portal link, and then > add variations of the parameters: > > [Captive Portal] [Wifi] [Wifi (with non-default wifi name)] [Non-Wifi] Done. https://codereview.chromium.org/1189413003/diff/20001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:23: <a href='captiveportal?url=https://google.com&landing_page=https://captive.porta... (On Wifi "CoffeeShopWifi")</a><br> On 2015/06/19 21:23:18, Mustafa Emre Acer wrote: > On 2015/06/19 21:09:03, bhanudev wrote: > > Are the labels good, or should I change them. Thanks. > > How about making this a bit fancier? Add a default captive portal link, and then > add variations of the parameters: > > [Captive Portal] [Wifi] [Wifi (with non-default wifi name)] [Non-Wifi] Done. https://codereview.chromium.org/1189413003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): https://codereview.chromium.org/1189413003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:61: const bool is_wifi_connection_; On 2015/06/19 21:23:18, Mustafa Emre Acer wrote: > no need for const for bool. Done. https://codereview.chromium.org/1189413003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:65: }; On 2015/06/19 21:23:18, Mustafa Emre Acer wrote: > On 2015/06/19 21:09:03, bhanudev wrote: > > I am not sure if this is the correct place to place the definition of a class, > > since this is only associated with the CreateCaptivePortalBlockingPage > function > > and not others. > > Looks okay, it's obvious from the base class name where this will be used. Done. https://codereview.chromium.org/1189413003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:162: bool is_wifi_connection = 0; On 2015/06/19 21:23:19, Mustafa Emre Acer wrote: > false instead of 0. Done.
Lgtm once you address the html comment. https://codereview.chromium.org/1189413003/diff/40001/chrome/browser/resource... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/1189413003/diff/40001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:22: <a href='captiveportal?url=https://google.com&landing_page=https://captive.porta... (Captive Portal)</a><br> Is this not the same as non-wifi? So you have three options in fact: Captive portal: [Non-WiFi] [WiFi] [WiFi with SSID] https://codereview.chromium.org/1189413003/diff/40001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.h (right): https://codereview.chromium.org/1189413003/diff/40001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:60: // Should only used for testing and chrome://interstitials page "Should only be used". Add a period at the end of the comment. https://codereview.chromium.org/1189413003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): https://codereview.chromium.org/1189413003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:166: // considered a wifi connection, even if is_wifi_connection is false nit: Period at the end of the comment.
https://codereview.chromium.org/1189413003/diff/40001/chrome/browser/resource... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/1189413003/diff/40001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:22: <a href='captiveportal?url=https://google.com&landing_page=https://captive.porta... (Captive Portal)</a><br> On 2015/06/19 22:50:37, Mustafa Emre Acer wrote: > Is this not the same as non-wifi? So you have three options in fact: > > Captive portal: > [Non-WiFi] [WiFi] [WiFi with SSID] Done. https://codereview.chromium.org/1189413003/diff/40001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.h (right): https://codereview.chromium.org/1189413003/diff/40001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:60: // Should only used for testing and chrome://interstitials page On 2015/06/19 22:50:37, Mustafa Emre Acer wrote: > "Should only be used". Add a period at the end of the comment. Done. https://codereview.chromium.org/1189413003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): https://codereview.chromium.org/1189413003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:166: // considered a wifi connection, even if is_wifi_connection is false On 2015/06/19 22:50:37, Mustafa Emre Acer wrote: > nit: Period at the end of the comment. Done.
bhanudev@google.com changed reviewers: + bauerb@chromium.org, xiyuan@chromium.org
On 2015/06/19 23:21:33, bhanudev wrote: > https://codereview.chromium.org/1189413003/diff/40001/chrome/browser/resource... > File chrome/browser/resources/security_warnings/interstitial_ui.html (right): > > https://codereview.chromium.org/1189413003/diff/40001/chrome/browser/resource... > chrome/browser/resources/security_warnings/interstitial_ui.html:22: <a > http://href= > (Captive Portal)</a><br> > On 2015/06/19 22:50:37, Mustafa Emre Acer wrote: > > Is this not the same as non-wifi? So you have three options in fact: > > > > Captive portal: > > [Non-WiFi] [WiFi] [WiFi with SSID] > > Done. > > https://codereview.chromium.org/1189413003/diff/40001/chrome/browser/ssl/capt... > File chrome/browser/ssl/captive_portal_blocking_page.h (right): > > https://codereview.chromium.org/1189413003/diff/40001/chrome/browser/ssl/capt... > chrome/browser/ssl/captive_portal_blocking_page.h:60: // Should only used for > testing and chrome://interstitials page > On 2015/06/19 22:50:37, Mustafa Emre Acer wrote: > > "Should only be used". Add a period at the end of the comment. > > Done. > > https://codereview.chromium.org/1189413003/diff/40001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): > > https://codereview.chromium.org/1189413003/diff/40001/chrome/browser/ui/webui... > chrome/browser/ui/webui/interstitials/interstitial_ui.cc:166: // considered a > wifi connection, even if is_wifi_connection is false > On 2015/06/19 22:50:37, Mustafa Emre Acer wrote: > > nit: Period at the end of the comment. > > Done. +xiyuan, +bauerb For owners review. PTAL? Thanks.
https://codereview.chromium.org/1189413003/diff/60001/chrome/browser/resource... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/1189413003/diff/60001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:22: <a href='captiveportal?url=https://google.com&landing_page=https://captive.porta... (Captive Portal, Non-Wifi)</a><br> You already use the |url| and |landing_page| values as defaults, so you don't need to pass them here. That will also make it easier to fit this into 80 columns (break after the opening <a> if necessary).
https://codereview.chromium.org/1189413003/diff/60001/chrome/browser/resource... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/1189413003/diff/60001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:22: <a href='captiveportal?url=https://google.com&landing_page=https://captive.porta... (Captive Portal, Non-Wifi)</a><br> On 2015/06/22 08:13:41, Bernhard Bauer wrote: > You already use the |url| and |landing_page| values as defaults, so you don't > need to pass them here. That will also make it easier to fit this into 80 > columns (break after the opening <a> if necessary). Thank you for the suggestion. Yes, the |url| and |landing_page| are defaults, so its good to not pass those parameters. Also, I think, the chrome://interstitials would be used by developers and for testing, so the parameters would help them see how the interstitial changes with different url and landing pages. Thanks.
https://codereview.chromium.org/1189413003/diff/60001/chrome/browser/resource... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/1189413003/diff/60001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:22: <a href='captiveportal?url=https://google.com&landing_page=https://captive.porta... (Captive Portal, Non-Wifi)</a><br> On 2015/06/22 17:41:57, bhanudev wrote: > On 2015/06/22 08:13:41, Bernhard Bauer wrote: > > You already use the |url| and |landing_page| values as defaults, so you don't > > need to pass them here. That will also make it easier to fit this into 80 > > columns (break after the opening <a> if necessary). > > Thank you for the suggestion. Yes, the |url| and |landing_page| are defaults, so > its good to not pass those parameters. > > Also, I think, the chrome://interstitials would be used by developers and for > testing, so the parameters would help them see how the interstitial changes with > different url and landing pages. Thanks. I think what Bernhard meant is that you can leave the parameters out from the first link, since it's just the defaults. The next two links already have the parameters so anyone using the page can see them (you can change the params to non-defaults (e.g. url to example.com) in the next two links).
https://codereview.chromium.org/1189413003/diff/60001/chrome/browser/resource... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/1189413003/diff/60001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:22: <a href='captiveportal?url=https://google.com&landing_page=https://captive.porta... (Captive Portal, Non-Wifi)</a><br> On 2015/06/22 17:47:03, Mustafa Emre Acer wrote: > On 2015/06/22 17:41:57, bhanudev wrote: > > On 2015/06/22 08:13:41, Bernhard Bauer wrote: > > > You already use the |url| and |landing_page| values as defaults, so you > don't > > > need to pass them here. That will also make it easier to fit this into 80 > > > columns (break after the opening <a> if necessary). > > > > Thank you for the suggestion. Yes, the |url| and |landing_page| are defaults, > so > > its good to not pass those parameters. > > > > Also, I think, the chrome://interstitials would be used by developers and for > > testing, so the parameters would help them see how the interstitial changes > with > > different url and landing pages. Thanks. > > I think what Bernhard meant is that you can leave the parameters out from the > first link, since it's just the defaults. The next two links already have the > parameters so anyone using the page can see them (you can change the params to > non-defaults (e.g. url to http://example.com) in the next two links). Done. https://codereview.chromium.org/1189413003/diff/60001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:22: <a href='captiveportal?url=https://google.com&landing_page=https://captive.porta... (Captive Portal, Non-Wifi)</a><br> On 2015/06/22 17:47:03, Mustafa Emre Acer wrote: > On 2015/06/22 17:41:57, bhanudev wrote: > > On 2015/06/22 08:13:41, Bernhard Bauer wrote: > > > You already use the |url| and |landing_page| values as defaults, so you > don't > > > need to pass them here. That will also make it easier to fit this into 80 > > > columns (break after the opening <a> if necessary). > > > > Thank you for the suggestion. Yes, the |url| and |landing_page| are defaults, > so > > its good to not pass those parameters. > > > > Also, I think, the chrome://interstitials would be used by developers and for > > testing, so the parameters would help them see how the interstitial changes > with > > different url and landing pages. Thanks. > > I think what Bernhard meant is that you can leave the parameters out from the > first link, since it's just the defaults. The next two links already have the > parameters so anyone using the page can see them (you can change the params to > non-defaults (e.g. url to http://example.com) in the next two links). Done. Sorry, I thought it the other way. Please let me know about improvements, if any. Thanks.
https://codereview.chromium.org/1189413003/diff/80001/chrome/browser/resource... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/1189413003/diff/80001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:23: <a href='captiveportal?url=https://example.com&landing_page=https://captive.port... (Captive Portal, on some Wifi)</a><br> Just remove the |url| parameter, so it's google.com. There's no difference I can see between the two URLs that would be of interest to a developer. Also, remove the |landing_page| parameter.
https://codereview.chromium.org/1189413003/diff/80001/chrome/browser/resource... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/1189413003/diff/80001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:23: <a href='captiveportal?url=https://example.com&landing_page=https://captive.port... (Captive Portal, on some Wifi)</a><br> On 2015/06/22 19:04:36, Bernhard Bauer wrote: > Just remove the |url| parameter, so it's http://google.com. There's no difference I can > see between the two URLs that would be of interest to a developer. Also, remove > the |landing_page| parameter. Done.
webui/* LGTM
https://codereview.chromium.org/1189413003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/1189413003/diff/120001/chrome/browser/resourc... chrome/browser/resources/security_warnings/interstitial_ui.html:23: <a href='captiveportal?is_wifi=1'>google.com (Captive Portal, on some Wifi)</a><br> Break this line after the <a> so it fits into 80 columns. Same for the next line.
https://codereview.chromium.org/1189413003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/1189413003/diff/120001/chrome/browser/resourc... chrome/browser/resources/security_warnings/interstitial_ui.html:23: <a href='captiveportal?is_wifi=1'>google.com (Captive Portal, on some Wifi)</a><br> On 2015/06/22 22:02:49, Bernhard Bauer wrote: > Break this line after the <a> so it fits into 80 columns. Same for the next > line. Done.
https://codereview.chromium.org/1189413003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/1189413003/diff/140001/chrome/browser/resourc... chrome/browser/resources/security_warnings/interstitial_ui.html:24: google.com (Captive Portal, on some Wifi)</a><br> Indent this line by two spaces, then break before the closing </a>, which should be aligned with the opening <a> again. Essentially, you are not breaking a single long line, you're opening and closing a new scope, so everything inside that scope is indented two spaces. FTR, the Chromium style guidelines for Web UI are here: http://dev.chromium.org/developers/web-development-style-guide (and refering to the Google HTML/CSS style guidelines, which are here: http://google-styleguide.googlecode.com/svn/trunk/htmlcssguide.xml).
https://codereview.chromium.org/1189413003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/1189413003/diff/140001/chrome/browser/resourc... chrome/browser/resources/security_warnings/interstitial_ui.html:24: google.com (Captive Portal, on some Wifi)</a><br> On 2015/06/23 08:48:47, Bernhard Bauer wrote: > Indent this line by two spaces, then break before the closing </a>, which should > be aligned with the opening <a> again. Essentially, you are not breaking a > single long line, you're opening and closing a new scope, so everything inside > that scope is indented two spaces. FTR, the Chromium style guidelines for Web UI > are here: http://dev.chromium.org/developers/web-development-style-guide (and > refering to the Google HTML/CSS style guidelines, which are here: > http://google-styleguide.googlecode.com/svn/trunk/htmlcssguide.xml). Done. Thank you for the details about scope..
lgtm
The CQ bit was checked by bhanudev@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from meacer@chromium.org, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/1189413003/#ps160001 (title: "Html Style Changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189413003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The trybot failed because of recent changes, which are not rebased into my local repo: 1. StartsWith[ASCII] is moved to base namespace recently, 2. New presubmit checks are added recently, which says "Do not use <br>; place blocking elements (<div>) as appropriate." PTAL? Thanks.
Still lgtm.
LGTM with one nit: https://codereview.chromium.org/1189413003/diff/220001/chrome/browser/resourc... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/1189413003/diff/220001/chrome/browser/resourc... chrome/browser/resources/security_warnings/interstitial_ui.html:7: <div> You don't need this div; you only need one around text runs, to make them into block elements.
https://codereview.chromium.org/1189413003/diff/220001/chrome/browser/resourc... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/1189413003/diff/220001/chrome/browser/resourc... chrome/browser/resources/security_warnings/interstitial_ui.html:7: <div> On 2015/06/24 08:00:12, Bernhard Bauer wrote: > You don't need this div; you only need one around text runs, to make them into > block elements. Done.
The CQ bit was checked by bhanudev@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, bauerb@chromium.org, meacer@chromium.org Link to the patchset: https://codereview.chromium.org/1189413003/#ps240001 (title: "html style changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189413003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
On 2015/06/25 19:26:39, commit-bot: I haz the power wrote: > Exceeded global retry quota Captive portal is not enabled in android builds. Added necessary |if defined()|s to handles that issue.
The CQ bit was checked by bhanudev@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, xiyuan@chromium.org, meacer@chromium.org Link to the patchset: https://codereview.chromium.org/1189413003/#ps280001 (title: "fixing crash for android builds-2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189413003/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/0c34909aa92c44020f8fdc4a4a1676b6d5d8fcd0 Cr-Commit-Position: refs/heads/master@{#338409} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
