Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
208683007:
Update incognito and guest browsing new tab page: (Closed)
|
Created:
6 years, 9 months ago by edwardjung Modified:
6 years, 7 months ago CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, estade+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, pedrosimonetti+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionUpdate incognito and guest browsing new tab page:
+ Updated styling.
+ Add new incognito man illustration.
+ Simplify the copy. Remove the extensions block.
BUG=356226
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266847
Patch Set 1 #Patch Set 2 : Updated typography and margins #Patch Set 3 : Tweaked margins following smaller heading size #Patch Set 4 : Add PH tags to the message strings #Patch Set 5 : Move image files to chrome/browser/resources/ntp4/images/ per oshima #Patch Set 6 : Move location of new incognito icons. #Patch Set 7 : Fix issue with corrupted images. #
Total comments: 2
Patch Set 8 : Removing images on the advice of estade #Patch Set 9 : Adjusted link colour to pass accessibility audit #Patch Set 10 : Revert back to using default system UI font. #Patch Set 11 : Wrap to 80 chars #
Total comments: 24
Patch Set 12 : Created a shared guest / incognito tab stylesheet, fixes to styles. #
Total comments: 4
Patch Set 13 : Refactor the structure of the HTML template to simplify message string construction. #Patch Set 14 : Fix ChromeOS build error, remove surplus font attribute #
Total comments: 11
Patch Set 15 : Made template string names more descriptive #Patch Set 16 : Update ChromeOS Guest session description ID #
Total comments: 2
Patch Set 17 : Fix nits #Patch Set 18 : Shorten illustration filename to fix presubmit #Messages
Total messages: 82 (0 generated)
An update to the style of the incognito and guest NTPs. Screenshots of the built pages are in the bug thread.
You probably should put these images in chrome/browser/resources/ntp4/images ? chrome/app/theme is for images used in C++ (and for the case web ui needs to use the same png files)
On 2014/04/11 13:07:05, oshima (OOO April 11 - 21) wrote: > You probably should put these images in > chrome/browser/resources/ntp4/images ? > > chrome/app/theme is for images used in C++ (and for the case web ui needs to use > the same png files) Thanks for explaining. I've moved the images to the suggested location.
I think you have to add the images in a separate CL in order to use the CQ for the rest of the patch. https://codereview.chromium.org/208683007/diff/150001/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_tab.html (left): https://codereview.chromium.org/208683007/diff/150001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.html:16: i18n-values=".style.fontFamily:fontfamily;.style.fontSize:fontsize"> why did you remove the font style?
Thanks, I'll create a new CL with the images shortly. https://codereview.chromium.org/208683007/diff/150001/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_tab.html (left): https://codereview.chromium.org/208683007/diff/150001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.html:16: i18n-values=".style.fontFamily:fontfamily;.style.fontSize:fontsize"> On 2014/04/11 22:15:09, Evan Stade wrote: > why did you remove the font style? We're standardising the font to Arial across the board in order to make it consistent with the standard NTP, rather than using the default UI font on different platforms. The font is now defined in the CSS.
On 2014/04/11 22:56:39, edwardjung wrote: > Thanks, I'll create a new CL with the images shortly. > > https://codereview.chromium.org/208683007/diff/150001/chrome/browser/resource... > File chrome/browser/resources/ntp4/incognito_tab.html (left): > > https://codereview.chromium.org/208683007/diff/150001/chrome/browser/resource... > chrome/browser/resources/ntp4/incognito_tab.html:16: > i18n-values=".style.fontFamily:fontfamily;.style.fontSize:fontsize"> > On 2014/04/11 22:15:09, Evan Stade wrote: > > why did you remove the font style? > > We're standardising the font to Arial across the board in order to make it > consistent with the standard NTP, rather than using the default UI font on > different platforms. The font is now defined in the CSS. That sounds like a bad idea. It's inconsistent with every other webui page. Why do you desire consistency with the normal NTP? I don't think normal and guest mode NTP are likely to be juxtaposed very often.
At the moment all of the web UI is inconsistent in it's use of typography. Other pages do already use Arial, like the 'Ooops', 'Offline' and 'Not available'. It's a start to making all of the web UI more consistent and beautiful. So the Settings pages will also be updated in future. The design has been been through Chrome UX review and has been approved by Sebastian and Glen. I've added Hannah from Chrome UX if you need more context or want to raise an objection. From a general point of view of organisation, the font should ideally set in the stylesheet separating the presentation styling from the page content. Inline CSS values are bad as it will ignore any adjustments from a stylesheet unless you resorted to the use of '!important'.
On 2014/04/14 10:09:06, edwardjung wrote: > At the moment all of the web UI is inconsistent in it's use of typography. Other > pages do already use Arial, like the 'Ooops', 'Offline' and 'Not available'. Those are all pages you never intentionally navigate to, but land on while attempting to navigate to a webpage. So in my mind, those are mimicking a web page, whereas settings (and most other webui pages that you choose to go to) are mimicking native UI. > It's a start to making all of the web UI more consistent and beautiful. So the > Settings pages will also be updated in future. > > The design has been been through Chrome UX review and has been approved by > Sebastian and Glen. I've added Hannah from Chrome UX if you need more context or > want to raise an objection. I would like some more context on this decision, yes. It always would have been much easier to "just use Arial everywhere". But we previously decided to take the more difficult path of matching the system. Chrome stands out when it's the only application that ignores my system font settings. If consistency is a goal, this is a step in the wrong direction. So I wonder what changed that made us reverse the previous decision about the desired behavior? > > From a general point of view of organisation, the font should ideally set in the > stylesheet separating the presentation styling from the page content. Inline CSS > values are bad as it will ignore any adjustments from a stylesheet unless you > resorted to the use of '!important'. Generally true, but it depends on how a property is inherited. In this case it wasn't set in the stylesheet because of the mechanism by which the system-appropriate font is injected into the page.
Just spoke with Sebastien - we should go with Native UI here. Sorry for the confusion Edward, that was my oversight.
> Just spoke with Sebastien - we should go with Native UI here. Okay, I've reverted the change to the inline system font definition. Evan, you should be good to continue with the review. Thanks. > Sorry for the confusion Edward, that was my oversight. No problem Hannah.
looks like guest_tab.css and incognito_tab.css are almost identical. Can you just use the same file for both, then have a separate file for the additions for incognito (i.e. the icon stuff)? https://codereview.chromium.org/208683007/diff/220001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/208683007/diff/220001/chrome/app/generated_re... chrome/app/generated_resources.grd:11264: + <ph name="BEGIN_P"><p></ph><ph name="BEGIN_BOLD"><strong></ph>Going incognito doesn’t hide your browsing from your employer, your internet service provider, or the websites you visit.<ph name="END_BOLD"></strong></ph><ph name="END_P"></p></ph> this is so hard to read and easy for translators to screw up. Is there any amount of the markup you can move into the html? For example it looks like the link text can just be text, and the <a href="..."> bit can be part of the html file. (It seems like we should be able to use the same link for all locales given that the UI language should be sent to the google support server via an HTML header). https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_tab.css (right): https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:6: background: #fafafa !important; if you don't want to respect the theme bg color, you should just remove that from new_incognito_tab_theme.css rather than !important-overriding it. https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:10: font-family: Arial, sans-serif; still need to remove this https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:15: color: #000; prefer 'black' but also, I don't think you need this at all --- black is inherited from .content https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:16: font-size: 1.7em !important; you don't need this !important. font-size: em already changes the size relative to what it would otherwise be. https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:19: margin: 0 0 .6em; If you made this just margin-top: 0, the only difference would be 0.07em more margin on the bottom. Is that 0.07em important? https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:32: margin: 0 0 1em; you don't need this, it doesn't do anything (default is already 1em above and below, and stacking margins get collapsed) https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:50: margin: 4px 35px 10px 0; why is this not the inverse of the ltr case? also, you can use -webkit-margin-start: and -webkit-margin-end: so you don't need separate rtl/ltr rules https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:54: background-color: #fff; prefer 'white' https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:60: margin: 5.5em auto 0; this can just be margin-top: 5.5em;
Thanks for the quick review. Created a new shared stylesheet. I haven't addressed the string simplification issue yet as I wasn't sure of the best implementation. https://codereview.chromium.org/208683007/diff/220001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/208683007/diff/220001/chrome/app/generated_re... chrome/app/generated_resources.grd:11264: + <ph name="BEGIN_P"><p></ph><ph name="BEGIN_BOLD"><strong></ph>Going incognito doesn’t hide your browsing from your employer, your internet service provider, or the websites you visit.<ph name="END_BOLD"></strong></ph><ph name="END_P"></p></ph> On 2014/04/16 22:10:57, Evan Stade wrote: >For example it looks like the > link text can just be text, and the <a href="..."> bit can be part of the html > file. (It seems like we should be able to use the same link for all locales > given that the UI language should be sent to the google support server via an > HTML header). How would I achieve this, as the link is part of the paragraph. Would I need to create multiple new message IDs? So for example the HTML could be like the following with hardcoded link text: <div class="content" i18n-values=".style.fontFamily:fontfamily;.style.fontSize:fontsize"> <div class="icon"></div> <h1 i18n-values=".innerHTML:heading"></h1> <p> <span i18n-values=".innerHTML:content1"></span> <a href="https://support.google.com/chrome/answer/95464" i18n-values=".innerHTML:learnMore"></a> </p> <p i18n-values=".innerHTML:content2"></p> </div> Then in ntp_resource_cache.cc I would set these individually. localized_strings.SetString("content1", ); localized_strings.SetString("learnMore", ); … Any guidance appreciated, I'm new to the code base. https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_tab.css (right): https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:6: background: #fafafa !important; On 2014/04/16 22:10:57, Evan Stade wrote: > if you don't want to respect the theme bg color, you should just remove that > from new_incognito_tab_theme.css rather than !important-overriding it. Done. https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:10: font-family: Arial, sans-serif; On 2014/04/16 22:10:57, Evan Stade wrote: > still need to remove this Done. https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:15: color: #000; On 2014/04/16 22:10:57, Evan Stade wrote: > prefer 'black' > > but also, I don't think you need this at all --- black is inherited from > .content Done. https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:16: font-size: 1.7em !important; On 2014/04/16 22:10:57, Evan Stade wrote: > you don't need this !important. font-size: em already changes the size relative > to what it would otherwise be. Done. https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:19: margin: 0 0 .6em; On 2014/04/16 22:10:57, Evan Stade wrote: > If you made this just margin-top: 0, the only difference would be 0.07em more > margin on the bottom. Is that 0.07em important? Now that we're back to the system fonts it's a tiny difference so removed. https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:32: margin: 0 0 1em; On 2014/04/16 22:10:57, Evan Stade wrote: > you don't need this, it doesn't do anything (default is already 1em above and > below, and stacking margins get collapsed) Done. https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:50: margin: 4px 35px 10px 0; On 2014/04/16 22:10:57, Evan Stade wrote: > why is this not the inverse of the ltr case? > > also, you can use -webkit-margin-start: and -webkit-margin-end: so you don't > need separate rtl/ltr rules My oversight. Thanks for the -webkit-margin tip. https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:54: background-color: #fff; On 2014/04/16 22:10:57, Evan Stade wrote: > prefer 'white' Done. https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:60: margin: 5.5em auto 0; On 2014/04/16 22:10:57, Evan Stade wrote: > this can just be margin-top: 5.5em; Without the auto left and right margins, the content container will not be centred. I collapsed the existing margin left and right into a single declaration. Is the general style to use individual declarations?
https://codereview.chromium.org/208683007/diff/220001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/208683007/diff/220001/chrome/app/generated_re... chrome/app/generated_resources.grd:11264: + <ph name="BEGIN_P"><p></ph><ph name="BEGIN_BOLD"><strong></ph>Going incognito doesn’t hide your browsing from your employer, your internet service provider, or the websites you visit.<ph name="END_BOLD"></strong></ph><ph name="END_P"></p></ph> On 2014/04/17 13:07:07, edwardjung wrote: > On 2014/04/16 22:10:57, Evan Stade wrote: > >For example it looks like the > > link text can just be text, and the <a href="..."> bit can be part of the html > > file. (It seems like we should be able to use the same link for all locales > > given that the UI language should be sent to the google support server via an > > HTML header). > > How would I achieve this, as the link is part of the paragraph. Would I need to > create multiple new message IDs? > > So for example the HTML could be like the following with hardcoded link text: > > <div class="content" > i18n-values=".style.fontFamily:fontfamily;.style.fontSize:fontsize"> > <div class="icon"></div> > <h1 i18n-values=".innerHTML:heading"></h1> > <p> > <span i18n-values=".innerHTML:content1"></span> > <a href="https://support.google.com/chrome/answer/95464" > i18n-values=".innerHTML:learnMore"></a> > </p> > <p i18n-values=".innerHTML:content2"></p> > </div> > > Then in ntp_resource_cache.cc I would set these individually. > > localized_strings.SetString("content1", ); > localized_strings.SetString("learnMore", ); > … > > Any guidance appreciated, I'm new to the code base. What you have looks about right. You can use i18n-content="FOOBAR" instead of i18n-values=".innerHTML:FOOBAR", assuming there's just plaintext and no markup in FOOBAR. One issue is that you have a period after the link. If it's possible to either linkify that period, or remove it completely, that would simplify things (then you don't need markup in the grd file). I think you can remove it because in chrome://settings, links don't have periods at the end, except in the case where there's a sentence that is only partially linkified. For example, "Set which search engine is used when searching from the |omnibox|." but "[ ] Offer to save your passwords. |Manage saved passwords|" (bars = linkified) The second paragraph is all strong, so you can handle that in CSS and remove the markup here. The first <p> is only partly strong unfortunately, so you have to leave a little bit of markup in the grd. https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_tab.css (right): https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:60: margin: 5.5em auto 0; On 2014/04/17 13:07:07, edwardjung wrote: > On 2014/04/16 22:10:57, Evan Stade wrote: > > this can just be margin-top: 5.5em; > > Without the auto left and right margins, the content container will not be > centred. I collapsed the existing margin left and right into a single > declaration. Is the general style to use individual declarations? either way is fine. I guess I prefer individual because it's easier to read (I can never quite remember the order/rules for 3/4 margin values). https://codereview.chromium.org/208683007/diff/210007/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_guest_tab.css (right): https://codereview.chromium.org/208683007/diff/210007/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_guest_tab.css:10: font-family: Arial, sans-serif; this is still here https://codereview.chromium.org/208683007/diff/210007/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_guest_tab.css:15: color: #000; this is still here
I've restructured the HTML and strings. The learn more link for incognito cannot be hard coded as it's a different link depending on if it's ChromeOS / Chrome. I found an example for link substitution in the template which works. https://codereview.chromium.org/208683007/diff/220001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/208683007/diff/220001/chrome/app/generated_re... chrome/app/generated_resources.grd:11264: + <ph name="BEGIN_P"><p></ph><ph name="BEGIN_BOLD"><strong></ph>Going incognito doesn’t hide your browsing from your employer, your internet service provider, or the websites you visit.<ph name="END_BOLD"></strong></ph><ph name="END_P"></p></ph> On 2014/04/17 16:25:07, Evan Stade wrote: > On 2014/04/17 13:07:07, edwardjung wrote: > > On 2014/04/16 22:10:57, Evan Stade wrote: > > >For example it looks like the > > > link text can just be text, and the <a href="..."> bit can be part of the > html > > > file. (It seems like we should be able to use the same link for all locales > > > given that the UI language should be sent to the google support server via > an > > > HTML header). > > > > How would I achieve this, as the link is part of the paragraph. Would I need > to > > create multiple new message IDs? > > > > So for example the HTML could be like the following with hardcoded link text: > > > > <div class="content" > > i18n-values=".style.fontFamily:fontfamily;.style.fontSize:fontsize"> > > <div class="icon"></div> > > <h1 i18n-values=".innerHTML:heading"></h1> > > <p> > > <span i18n-values=".innerHTML:content1"></span> > > <a href="https://support.google.com/chrome/answer/95464" > > i18n-values=".innerHTML:learnMore"></a> > > </p> > > <p i18n-values=".innerHTML:content2"></p> > > </div> > > > > Then in ntp_resource_cache.cc I would set these individually. > > > > localized_strings.SetString("content1", ); > > localized_strings.SetString("learnMore", ); > > … > > > > Any guidance appreciated, I'm new to the code base. > > What you have looks about right. > > You can use i18n-content="FOOBAR" instead of i18n-values=".innerHTML:FOOBAR", > assuming there's just plaintext and no markup in FOOBAR. > > One issue is that you have a period after the link. If it's possible to either > linkify that period, or remove it completely, that would simplify things (then > you don't need markup in the grd file). I think you can remove it because in > chrome://settings, links don't have periods at the end, except in the case where > there's a sentence that is only partially linkified. For example, "Set which > search engine is used when searching from the |omnibox|." but "[ ] Offer to save > your passwords. |Manage saved passwords|" (bars = linkified) > > The second paragraph is all strong, so you can handle that in CSS and remove the > markup here. The first <p> is only partly strong unfortunately, so you have to > leave a little bit of markup in the grd. Done. https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_tab.css (right): https://codereview.chromium.org/208683007/diff/220001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:60: margin: 5.5em auto 0; Good to know. Thanks https://codereview.chromium.org/208683007/diff/210007/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_guest_tab.css (right): https://codereview.chromium.org/208683007/diff/210007/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_guest_tab.css:10: font-family: Arial, sans-serif; On 2014/04/17 16:25:07, Evan Stade wrote: > this is still here Done. https://codereview.chromium.org/208683007/diff/210007/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_guest_tab.css:15: color: #000; On 2014/04/17 16:25:07, Evan Stade wrote: > this is still here Done. https://codereview.chromium.org/208683007/diff/270001/chrome/browser/ui/webui... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://codereview.chromium.org/208683007/diff/270001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:308: new_tab_link_ids = IDS_NEW_TAB_GUEST_SESSION_LEARN_MORE_LINK; Made changes here so that it's in line with the other templates. I've not been able to verify this page visually as I'm not sure when this page is displayed. The guest session on CrOS does not allow you to open an incognito window, only new guest tabs.
https://codereview.chromium.org/208683007/diff/270001/chrome/browser/resource... File chrome/browser/resources/chromeos/guest_session_tab.html (right): https://codereview.chromium.org/208683007/diff/270001/chrome/browser/resource... chrome/browser/resources/chromeos/guest_session_tab.html:27: <span i18n-content="message"></span> nit: can you make the name for the content a little more specific than "message"? ditto for "heading" https://codereview.chromium.org/208683007/diff/270001/chrome/browser/resource... File chrome/browser/resources/ntp4/guest_tab.html (right): https://codereview.chromium.org/208683007/diff/270001/chrome/browser/resource... chrome/browser/resources/ntp4/guest_tab.html:26: cr.define('ntp', function() { this seems overkill in this case, make it match the briefer version found in the otr ntp? https://codereview.chromium.org/208683007/diff/270001/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_guest_tab.css (right): https://codereview.chromium.org/208683007/diff/270001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_guest_tab.css:3: * found in the LICENSE file. */ nit: file-level comment ("this is the css shared by incognito NTP and guest NTP...") also, file name should be incognito_and_guest_ntp.css or something because currently it sounds like the intersection rather than union of incognito and guest modes. https://codereview.chromium.org/208683007/diff/270001/chrome/browser/ui/webui... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://codereview.chromium.org/208683007/diff/270001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:308: new_tab_link_ids = IDS_NEW_TAB_GUEST_SESSION_LEARN_MORE_LINK; On 2014/04/22 14:24:35, edwardjung wrote: > Made changes here so that it's in line with the other templates. I've not been > able to verify this page visually as I'm not sure when this page is displayed. > The guest session on CrOS does not allow you to open an incognito window, only > new guest tabs. You should ping someone (altimofeev looks like a good candidate) to figure out how to trigger guest incognito, or if it doesn't exist any more rip this code out.
I'll get to this tomorrow, but in the meantime had one question. https://codereview.chromium.org/208683007/diff/270001/chrome/browser/resource... File chrome/browser/resources/ntp4/guest_tab.html (right): https://codereview.chromium.org/208683007/diff/270001/chrome/browser/resource... chrome/browser/resources/ntp4/guest_tab.html:26: cr.define('ntp', function() { On 2014/04/22 17:52:52, Evan Stade wrote: > this seems overkill in this case, make it match the briefer version found in the > otr ntp? Not sure I understand, the JS used on the OTR NTP is identical, with an extra function for the bookmarks bar.
https://codereview.chromium.org/208683007/diff/270001/chrome/browser/resource... File chrome/browser/resources/ntp4/guest_tab.html (right): https://codereview.chromium.org/208683007/diff/270001/chrome/browser/resource... chrome/browser/resources/ntp4/guest_tab.html:26: cr.define('ntp', function() { On 2014/04/22 18:34:01, edwardjung wrote: > On 2014/04/22 17:52:52, Evan Stade wrote: > > this seems overkill in this case, make it match the briefer version found in > the > > otr ntp? > > Not sure I understand, the JS used on the OTR NTP is identical, with an extra > function for the bookmarks bar. sorry, guest_session_tab.html
https://codereview.chromium.org/208683007/diff/270001/chrome/browser/resource... File chrome/browser/resources/chromeos/guest_session_tab.html (right): https://codereview.chromium.org/208683007/diff/270001/chrome/browser/resource... chrome/browser/resources/chromeos/guest_session_tab.html:27: <span i18n-content="message"></span> On 2014/04/22 17:52:52, Evan Stade wrote: > nit: can you make the name for the content a little more specific than > "message"? ditto for "heading" Done. https://codereview.chromium.org/208683007/diff/270001/chrome/browser/resource... File chrome/browser/resources/ntp4/guest_tab.html (right): https://codereview.chromium.org/208683007/diff/270001/chrome/browser/resource... chrome/browser/resources/ntp4/guest_tab.html:26: cr.define('ntp', function() { On 2014/04/22 18:35:22, Evan Stade wrote: > On 2014/04/22 18:34:01, edwardjung wrote: > > On 2014/04/22 17:52:52, Evan Stade wrote: > > > this seems overkill in this case, make it match the briefer version found in > > the > > > otr ntp? > > > > Not sure I understand, the JS used on the OTR NTP is identical, with an extra > > function for the bookmarks bar. > > sorry, guest_session_tab.html Done. https://codereview.chromium.org/208683007/diff/270001/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_guest_tab.css (right): https://codereview.chromium.org/208683007/diff/270001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_guest_tab.css:3: * found in the LICENSE file. */ On 2014/04/22 17:52:52, Evan Stade wrote: > nit: file-level comment ("this is the css shared by incognito NTP and guest > NTP...") > > also, file name should be incognito_and_guest_ntp.css or something because > currently it sounds like the intersection rather than union of incognito and > guest modes. Done. https://codereview.chromium.org/208683007/diff/270001/chrome/browser/ui/webui... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://codereview.chromium.org/208683007/diff/270001/chrome/browser/ui/webui... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:308: new_tab_link_ids = IDS_NEW_TAB_GUEST_SESSION_LEARN_MORE_LINK; Confirmed with the Moscow team that this is still required. This enterprise and guest section is currently not triggered which they discovered is a bug. New bug has been filed, which I will fix separately in a new CL. https://code.google.com/p/chromium/issues/detail?id=365831
I don't think you need my review any more. removing me from reviews list.
lgtm https://codereview.chromium.org/208683007/diff/310001/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_and_guest_tab.css (right): https://codereview.chromium.org/208683007/diff/310001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_and_guest_tab.css:6: */ nit: \n
Thanks for the review Evan. https://codereview.chromium.org/208683007/diff/310001/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_and_guest_tab.css (right): https://codereview.chromium.org/208683007/diff/310001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_and_guest_tab.css:6: */ On 2014/04/24 00:38:21, Evan Stade wrote: > nit: \n Done.
Nikita, could you review the small update in guest_session_tab.html. Thanks.
On 2014/04/24 10:06:22, edwardjung wrote: > Nikita, could you review the small update in guest_session_tab.html. Thanks. lgtm
The CQ bit was checked by edwardjung@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edwardjung@chromium.org/208683007/330001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by edwardjung@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edwardjung@chromium.org/208683007/330001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by edwardjung@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edwardjung@chromium.org/208683007/330001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by edwardjung@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edwardjung@chromium.org/208683007/330001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by edwardjung@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edwardjung@chromium.org/208683007/330001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by edwardjung@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edwardjung@chromium.org/208683007/330001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by edwardjung@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edwardjung@chromium.org/208683007/330001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by edwardjung@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edwardjung@chromium.org/208683007/330001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by edwardjung@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edwardjung@chromium.org/208683007/330001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by edwardjung@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edwardjung@chromium.org/208683007/330001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by edwardjung@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edwardjung@chromium.org/208683007/330001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by edwardjung@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edwardjung@chromium.org/208683007/330001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by edwardjung@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edwardjung@chromium.org/208683007/350001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by edwardjung@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edwardjung@chromium.org/208683007/350001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium
The CQ bit was checked by edwardjung@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edwardjung@chromium.org/208683007/350001
Message was sent while issue was closed.
Change committed as 266847 |
