Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10827146:
crbug.com/127841 - Request Tablet Site on CB with touch screen. (Closed)
|
Created:
8 years, 4 months ago by sschmitz Modified:
8 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, Emmanuel Saint-loubert-Bié, Rick Byers Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
Descriptioncrbug.com/127841 - Request Tablet Site on CB with touch screen.
Allow user to request tablet site via a toggle on the wrench menu. Under about-flag & cmd line switch:
--enable-request-tablet-site
BUG=127841
TEST=Run with flag;
URLs to try: whatsmyuseragent.com, whatsmyua.com, slashdot.org, www.espn.com, www,facebook.com, www.google.com
toggle "Request tablet site" under wrench menu
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155872
Patch Set 1 #Patch Set 2 : removed spurious whitespace differences wrt master #
Total comments: 19
Patch Set 3 : set UA override for all entries; impl. reviewer comments #
Total comments: 12
Patch Set 4 : Disable RTS control when no tab or no nav. entry; impl. reviewer comments #
Total comments: 10
Patch Set 5 : impl. review comments #
Total comments: 4
Patch Set 6 : Changed to #Patch Set 7 : Rebase #
Total comments: 8
Patch Set 8 : use ReloadOriginalRequestURL instead of Reload; impl. review comments from sky #
Total comments: 8
Patch Set 9 : rebase and cleanup #Patch Set 10 : removed spurious comment #Patch Set 11 : include touch factory header only for chrome os #Patch Set 12 : removed spurious newline #
Total comments: 10
Patch Set 13 : fix for Dan's comments #
Total comments: 15
Patch Set 14 : fixed review items by sky@ #Patch Set 15 : removed unused flag (2nd rts flag) #
Total comments: 6
Patch Set 16 : fixed review issues #
Total comments: 2
Patch Set 17 : fix review issues by jamesr@ #Patch Set 18 : Rebasing #Messages
Total messages: 43 (0 generated)
Dan and Rick, thanks much for your help so far. Would you please do a prelim. review, in particular: - chrome/browser/ui/browser_commands.cc (followed Dan's suggestion) - chrome/browser/ui/toolbar/wrench_menu_model.cc - webkit/glue/user_agent.cc Is the following a good choice for overridden part in the user agent: "(Linux; U; Android 4.0.2; en-us; Galaxy Nexus Build/ICL53F)" Thanks much, Stefan
Thanks! I took a cursory pass at your CL. I'm kind of curious to see how the flag inheritance behavior in the NavigationEntry works on ChromeOS with this patch applied, but it looks like it should more or less work as the original email said it should. http://codereview.chromium.org/10827146/diff/2001/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_commands.cc (right): http://codereview.chromium.org/10827146/diff/2001/chrome/browser/ui/browser_c... chrome/browser/ui/browser_commands.cc:877: current_tab->SetUserAgentOverride(std::string()); I don't think you have to worry about un-setting the user agent override string here, unless you want to be doubly sure that the string doesn't get used. Setting the boolean to false should probably be enough. Also, if this is going to persist as a tab-level setting, you should iterate over all of the NavigationEntrys in the NavigationController and set the override flags accordingly. http://codereview.chromium.org/10827146/diff/2001/chrome/browser/ui/toolbar/w... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): http://codereview.chromium.org/10827146/diff/2001/chrome/browser/ui/toolbar/w... chrome/browser/ui/toolbar/wrench_menu_model.cc:519: // Remove this blank line http://codereview.chromium.org/10827146/diff/2001/chrome/browser/ui/toolbar/w... chrome/browser/ui/toolbar/wrench_menu_model.cc:532: switches::kEnableRequestTabletSiteEvenIfNonTouchDevice))) The indenting is kind of funky here. Consider separating out the checks here into separate bools? http://codereview.chromium.org/10827146/diff/2001/webkit/glue/user_agent.cc File webkit/glue/user_agent.cc (right): http://codereview.chromium.org/10827146/diff/2001/webkit/glue/user_agent.cc#n... webkit/glue/user_agent.cc:167: // nit: Remove line http://codereview.chromium.org/10827146/diff/2001/webkit/glue/user_agent.cc#n... webkit/glue/user_agent.cc:178: // Seems relatively fragile, but I don't have a better alternative at the moment. I filed http://crbug.com/128570 a while back because we're having trouble spoofing a desktop UA for Android, too... I could revisit this bit if I come up with a good solution for both of us. http://codereview.chromium.org/10827146/diff/2001/webkit/glue/user_agent.cc#n... webkit/glue/user_agent.cc:186: "(Linux; U; Android 4.0.2; en-us; Galaxy Nexus Build/ICL53F)"); Yank this literal out of the replace() call and make it a constant somewhere above it. Also, the user agent format was updated a while ago; check: https://developers.google.com/chrome/mobile/docs/user-agent http://codereview.chromium.org/10827146/diff/2001/webkit/glue/user_agent.h File webkit/glue/user_agent.h (right): http://codereview.chromium.org/10827146/diff/2001/webkit/glue/user_agent.h#ne... webkit/glue/user_agent.h:35: std::string BuildUserAgentOverrideForTabletSiteFromUserAgent( Add comment describing function.
Made changes according to Dan's review. I see some issues. Not sure what to do about them: 1. www.facebook.com correctly switches to tablet but does not switch back (on toggling back to RDS), because it has redirected to m.cnn... as described in: https://sites.google.com/a/google.com/clank/engineering/request-desktop-site 2. When exiting and restating Chrome, the bool state IsUAOverridden is remembered (per tab), but the actual UA override string is not. http://codereview.chromium.org/10827146/diff/2001/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_commands.cc (right): http://codereview.chromium.org/10827146/diff/2001/chrome/browser/ui/browser_c... chrome/browser/ui/browser_commands.cc:877: current_tab->SetUserAgentOverride(std::string()); On 2012/08/03 00:08:26, dfalcantara wrote: > I don't think you have to worry about un-setting the user agent override string > here, unless you want to be doubly sure that the string doesn't get used. > Setting the boolean to false should probably be enough. I agree, but I prefer to unset the string. It seems a bit more as defensive programming. > > Also, if this is going to persist as a tab-level setting, you should iterate > over all of the NavigationEntrys in the NavigationController and set the > override flags accordingly. Done http://codereview.chromium.org/10827146/diff/2001/chrome/browser/ui/toolbar/w... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): http://codereview.chromium.org/10827146/diff/2001/chrome/browser/ui/toolbar/w... chrome/browser/ui/toolbar/wrench_menu_model.cc:519: // On 2012/08/03 00:08:26, dfalcantara wrote: > Remove this blank line Done. http://codereview.chromium.org/10827146/diff/2001/chrome/browser/ui/toolbar/w... chrome/browser/ui/toolbar/wrench_menu_model.cc:532: switches::kEnableRequestTabletSiteEvenIfNonTouchDevice))) On 2012/08/03 00:08:26, dfalcantara wrote: > The indenting is kind of funky here. Consider separating out the checks here > into separate bools? Done. http://codereview.chromium.org/10827146/diff/2001/webkit/glue/user_agent.cc File webkit/glue/user_agent.cc (right): http://codereview.chromium.org/10827146/diff/2001/webkit/glue/user_agent.cc#n... webkit/glue/user_agent.cc:167: // On 2012/08/03 00:08:26, dfalcantara wrote: > nit: Remove line Done. http://codereview.chromium.org/10827146/diff/2001/webkit/glue/user_agent.cc#n... webkit/glue/user_agent.cc:178: // On 2012/08/03 00:08:26, dfalcantara wrote: > Seems relatively fragile, but I don't have a better alternative at the moment. > I filed http://crbug.com/128570 a while back because we're having trouble > spoofing a desktop UA for Android, too... I could revisit this bit if I come up > with a good solution for both of us. Yes. I agree. Thanks for looking out. http://codereview.chromium.org/10827146/diff/2001/webkit/glue/user_agent.cc#n... webkit/glue/user_agent.cc:186: "(Linux; U; Android 4.0.2; en-us; Galaxy Nexus Build/ICL53F)"); On 2012/08/03 00:08:26, dfalcantara wrote: > Yank this literal out of the replace() call and make it a constant somewhere > above it. Also, the user agent format was updated a while ago; check: > https://developers.google.com/chrome/mobile/docs/user-agent Done. http://codereview.chromium.org/10827146/diff/2001/webkit/glue/user_agent.h File webkit/glue/user_agent.h (right): http://codereview.chromium.org/10827146/diff/2001/webkit/glue/user_agent.h#ne... webkit/glue/user_agent.h:35: std::string BuildUserAgentOverrideForTabletSiteFromUserAgent( On 2012/08/03 00:08:26, dfalcantara wrote: > Add comment describing function. Done.
http://codereview.chromium.org/10827146/diff/2001/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/10827146/diff/2001/chrome/app/generated_resour... chrome/app/generated_resources.grd:1326: + <message name="IDS_TOGGLE_REQUEST_TABLET_SITE" desc="The toggle to request tablet site on a touch dev."> Add "In Title Case" to the desc for the sake of the translators. http://codereview.chromium.org/10827146/diff/2001/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_commands.cc (right): http://codereview.chromium.org/10827146/diff/2001/chrome/browser/ui/browser_c... chrome/browser/ui/browser_commands.cc:877: current_tab->SetUserAgentOverride(std::string()); On 2012/08/03 00:08:26, dfalcantara wrote: > I don't think you have to worry about un-setting the user agent override string > here, unless you want to be doubly sure that the string doesn't get used. > Setting the boolean to false should probably be enough. > > Also, if this is going to persist as a tab-level setting, you should iterate > over all of the NavigationEntrys in the NavigationController and set the > override flags accordingly. I'm not sure that's what we want. This is about scenarios like the following, right? 1. Go to a site 2. Click on a link 3. Turn on 'request tablet site' 4. Click back In that case I think we should be back in the state where the setting is off - just like Android. I may have over-simplified earlier saying that the setting should persist for the life of the tab. I meant just that when it's enabled and you enter a new URL in the omnibox, it would still be enabled. I think it would be confusing for the user if the state wasn't preserved with back/forward. http://codereview.chromium.org/10827146/diff/7002/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_commands.cc (right): http://codereview.chromium.org/10827146/diff/7002/chrome/browser/ui/browser_c... chrome/browser/ui/browser_commands.cc:872: if (!current_tab) return; newline before return If there's no current tab, can we grey out the command rather than silently have it do nothing? Feel free to worry about this in a future CL instead of fixing now since this feature is still experimental (leave a TODO referencing some bug to track it). http://codereview.chromium.org/10827146/diff/7002/chrome/browser/ui/browser_c... chrome/browser/ui/browser_commands.cc:875: if (!entry) return; If there's no active entry we should probably still be toggling a state somewhere I think. I'm not sure when exactly this is null (on a new blank tab perhaps?), but whenever it is, if the user clicks the setting and then types in a URL, they will expect the setting to take effect. Again, this is OK as is for experimentation reasons, but add a TODO comment somewhere referencing the bug the tracks this. http://codereview.chromium.org/10827146/diff/7002/chrome/browser/ui/toolbar/w... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): http://codereview.chromium.org/10827146/diff/7002/chrome/browser/ui/toolbar/w... chrome/browser/ui/toolbar/wrench_menu_model.cc:367: if (!browser_) return false; newline after if, here and elsewhere http://codereview.chromium.org/10827146/diff/7002/chrome/browser/ui/toolbar/w... chrome/browser/ui/toolbar/wrench_menu_model.cc:373: return entry->GetIsOverridingUserAgent(); Again, I think you want some independent state somewhere for this checkbox - maybe just here in the WrenchMenuModel. It needs to be updated from the NavigationEntry on navigation (due to back/forward), but should be able to be set true even when there is no current NavigationEntry (but needs to affect new NavigationEntries). http://codereview.chromium.org/10827146/diff/7002/chrome/browser/ui/toolbar/w... chrome/browser/ui/toolbar/wrench_menu_model.cc:530: (enableFlag && ui::TouchFactory::GetInstance()->IsTouchDevicePresent())) IsTouchDevicePresent is itself behind a flag (--enable-touch-events) which is still off by default (expected to be turned on in M23). If you like you can eliminate your enableRequestTabletSite (relying implicitly just on --enable-touch-events). That'll save you the hassle of turning this flag on in places where we're already testing our experimental touch screen support, and save you from having to remove this flag later. http://codereview.chromium.org/10827146/diff/7002/webkit/glue/user_agent.cc File webkit/glue/user_agent.cc (right): http://codereview.chromium.org/10827146/diff/7002/webkit/glue/user_agent.cc#n... webkit/glue/user_agent.cc:33: "(Linux; Android 4.0.4; Galaxy Nexus Build/IMM76B)"; This string looks fine to me.
Update wrt Rick's comments. Thanks much. http://codereview.chromium.org/10827146/diff/2001/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/10827146/diff/2001/chrome/app/generated_resour... chrome/app/generated_resources.grd:1326: + <message name="IDS_TOGGLE_REQUEST_TABLET_SITE" desc="The toggle to request tablet site on a touch dev."> On 2012/08/03 15:53:06, Rick Byers wrote: > Add "In Title Case" to the desc for the sake of the translators. Done. http://codereview.chromium.org/10827146/diff/2001/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_commands.cc (right): http://codereview.chromium.org/10827146/diff/2001/chrome/browser/ui/browser_c... chrome/browser/ui/browser_commands.cc:877: current_tab->SetUserAgentOverride(std::string()); On 2012/08/03 15:53:06, Rick Byers wrote: > On 2012/08/03 00:08:26, dfalcantara wrote: > > I don't think you have to worry about un-setting the user agent override > string > > here, unless you want to be doubly sure that the string doesn't get used. > > Setting the boolean to false should probably be enough. > > > > Also, if this is going to persist as a tab-level setting, you should iterate > > over all of the NavigationEntrys in the NavigationController and set the > > override flags accordingly. > > I'm not sure that's what we want. This is about scenarios like the following, > right? > 1. Go to a site > 2. Click on a link > 3. Turn on 'request tablet site' > 4. Click back > > In that case I think we should be back in the state where the setting is off - > just like Android. I may have over-simplified earlier saying that the setting > should persist for the life of the tab. I meant just that when it's enabled and > you enter a new URL in the omnibox, it would still be enabled. I think it would > be confusing for the user if the state wasn't preserved with back/forward. Yes, good question. I can see benefits for both scenarios "per tab (all entries)" vs. "per current entry only". I'm leaning towards the "Per Tab (for all entries)" scenario. Once the user toggles "RTS" to on, it "sticks" for the current tab. I think this may be more intuitive. I was already surprised to learn that "wrench menu settings" can be for the current tab only. I had erroneously assumed it applies for all tabs (even windows) from "then on". Therefore, to make a "wrench menu setting" (in this case "RTS") even more fine grained may be unintuitive for some users. http://codereview.chromium.org/10827146/diff/7002/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_commands.cc (right): http://codereview.chromium.org/10827146/diff/7002/chrome/browser/ui/browser_c... chrome/browser/ui/browser_commands.cc:872: if (!current_tab) return; On 2012/08/03 15:53:06, Rick Byers wrote: > newline before return > > If there's no current tab, can we grey out the command rather than silently have > it do nothing? Feel free to worry about this in a future CL instead of fixing > now since this feature is still experimental (leave a TODO referencing some bug > to track it). I made a code change elsewhere to disable the control (dimmed) in this case. I added a comment here to say so. Done. http://codereview.chromium.org/10827146/diff/7002/chrome/browser/ui/browser_c... chrome/browser/ui/browser_commands.cc:875: if (!entry) return; On 2012/08/03 15:53:06, Rick Byers wrote: > If there's no active entry we should probably still be toggling a state > somewhere I think. I'm not sure when exactly this is null (on a new blank tab > perhaps?), but whenever it is, if the user clicks the setting and then types in > a URL, they will expect the setting to take effect. > > Again, this is OK as is for experimentation reasons, but add a TODO comment > somewhere referencing the bug the tracks this. Same as above: control is disabled; comment in code Done. http://codereview.chromium.org/10827146/diff/7002/chrome/browser/ui/toolbar/w... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): http://codereview.chromium.org/10827146/diff/7002/chrome/browser/ui/toolbar/w... chrome/browser/ui/toolbar/wrench_menu_model.cc:367: if (!browser_) return false; On 2012/08/03 15:53:06, Rick Byers wrote: > newline after if, here and elsewhere Done. http://codereview.chromium.org/10827146/diff/7002/chrome/browser/ui/toolbar/w... chrome/browser/ui/toolbar/wrench_menu_model.cc:373: return entry->GetIsOverridingUserAgent(); On 2012/08/03 15:53:06, Rick Byers wrote: > Again, I think you want some independent state somewhere for this checkbox - > maybe just here in the WrenchMenuModel. It needs to be updated from the > NavigationEntry on navigation (due to back/forward), but should be able to be > set true even when there is no current NavigationEntry (but needs to affect new > NavigationEntries). Similar to browser_commands.cc, the control is disabled in this case and I added a comment here to say so. When the control is disabled the state is always "not checked". I hope that is ok, because then we can keep the state in only one place (in the navigation entry). Done. http://codereview.chromium.org/10827146/diff/7002/chrome/browser/ui/toolbar/w... chrome/browser/ui/toolbar/wrench_menu_model.cc:530: (enableFlag && ui::TouchFactory::GetInstance()->IsTouchDevicePresent())) On 2012/08/03 15:53:06, Rick Byers wrote: > IsTouchDevicePresent is itself behind a flag (--enable-touch-events) which is > still off by default (expected to be turned on in M23). If you like you can > eliminate your enableRequestTabletSite (relying implicitly just on > --enable-touch-events). That'll save you the hassle of turning this flag on in > places where we're already testing our experimental touch screen support, and > save you from having to remove this flag later. Question: Don't we want the added control to turn this feature (RTS) on/off independently when --enable-touch-events is on? So, for now I left it in. http://codereview.chromium.org/10827146/diff/7002/webkit/glue/user_agent.cc File webkit/glue/user_agent.cc (right): http://codereview.chromium.org/10827146/diff/7002/webkit/glue/user_agent.cc#n... webkit/glue/user_agent.cc:33: "(Linux; Android 4.0.4; Galaxy Nexus Build/IMM76B)"; On 2012/08/03 15:53:06, Rick Byers wrote: > This string looks fine to me. Done.
https://chromiumcodereview.appspot.com/10827146/diff/12002/chrome/browser/ui/... File chrome/browser/ui/browser_commands.cc (right): https://chromiumcodereview.appspot.com/10827146/diff/12002/chrome/browser/ui/... chrome/browser/ui/browser_commands.cc:873: return; // The control is disabled in this case Maybe put a general comment above about the control being disabled if we return early instead of repeating? https://chromiumcodereview.appspot.com/10827146/diff/12002/chrome/browser/ui/... chrome/browser/ui/browser_commands.cc:882: NavigationEntry* entryAtIdx = controller.GetEntryAtIndex(ii); Can't remember what the style guide says exactly, but IIRC we use _ instead of camel case for variable names. entryAtIdx -> entry_at_index, entryCount->entry_count, etc. https://chromiumcodereview.appspot.com/10827146/diff/12002/chrome/browser/ui/... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): https://chromiumcodereview.appspot.com/10827146/diff/12002/chrome/browser/ui/... chrome/browser/ui/toolbar/wrench_menu_model.cc:375: return false; // The control is disabled (dim) in this case Make a catch-all comment stating this instead of tripling it. https://chromiumcodereview.appspot.com/10827146/diff/12002/chrome/browser/ui/... chrome/browser/ui/toolbar/wrench_menu_model.cc:397: return false; Re-factor this check out into its own function since it's duplicated. https://chromiumcodereview.appspot.com/10827146/diff/12002/chrome/browser/ui/... chrome/browser/ui/toolbar/wrench_menu_model.cc:541: bool enableFlag = CommandLine::ForCurrentProcess()->HasSwitch( Style: don't use camel case
update based on Dan's review. Thanks much. http://codereview.chromium.org/10827146/diff/12002/chrome/browser/ui/browser_... File chrome/browser/ui/browser_commands.cc (right): http://codereview.chromium.org/10827146/diff/12002/chrome/browser/ui/browser_... chrome/browser/ui/browser_commands.cc:873: return; // The control is disabled in this case On 2012/08/03 20:13:26, dfalcantara wrote: > Maybe put a general comment above about the control being disabled if we return > early instead of repeating? Done. http://codereview.chromium.org/10827146/diff/12002/chrome/browser/ui/browser_... chrome/browser/ui/browser_commands.cc:882: NavigationEntry* entryAtIdx = controller.GetEntryAtIndex(ii); On 2012/08/03 20:13:26, dfalcantara wrote: > Can't remember what the style guide says exactly, but IIRC we use _ instead of > camel case for variable names. entryAtIdx -> entry_at_index, > entryCount->entry_count, etc. Done. http://codereview.chromium.org/10827146/diff/12002/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): http://codereview.chromium.org/10827146/diff/12002/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/wrench_menu_model.cc:375: return false; // The control is disabled (dim) in this case On 2012/08/03 20:13:26, dfalcantara wrote: > Make a catch-all comment stating this instead of tripling it. Done. http://codereview.chromium.org/10827146/diff/12002/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/wrench_menu_model.cc:397: return false; On 2012/08/03 20:13:26, dfalcantara wrote: > Re-factor this check out into its own function since it's duplicated. Done. http://codereview.chromium.org/10827146/diff/12002/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/wrench_menu_model.cc:541: bool enableFlag = CommandLine::ForCurrentProcess()->HasSwitch( On 2012/08/03 20:13:26, dfalcantara wrote: > Style: don't use camel case Done.
> 1. www.facebook.com correctly switches to tablet but does not switch back (on toggling back to RDS), because it has redirected to m.cnn... as described in: https://sites.google.com/a/google.com/clank/engineering/request-desktop-site Not sure why it'd switch to m.cnn.com... do you mean it gets stuck on m.facebook.com? Could I drop by and see this some time? > 2. When exiting and restating Chrome, the bool state IsUAOverridden is remembered (per tab), but the actual UA override string is not. This should actually be written out; there's even two unit tests to check this. Does ChromeOS not follow the typical session_service/tab_restore_service pathways? > I can see benefits for both scenarios "per tab (all entries)" vs. "per current entry only". I'm leaning towards the "Per Tab (for all entries)" scenario. Once the user toggles "RTS" to on, it "sticks" for the current tab. I think this may be more intuitive. FWIW, I think Bling does the former and Clank does the latter. Clank did it on a per-navigation basis because it made sense when navigating backwards: if you're using a per-tab setting, if you initially loaded a page with a desktop UA, navigate somewhere, flip the flag, and navigate back to the original page, the menu would indicate that it's using a tablet UA while showing the page loaded with a desktop UA until the page is reloaded. It seems like it'd be more confusing in that case. http://codereview.chromium.org/10827146/diff/12005/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): http://codereview.chromium.org/10827146/diff/12005/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/wrench_menu_model.cc:674: content::NavigationEntry* WrenchMenuModel::GetActiveNavigationEntry() const Style: Brace goes at the end of the previous line. http://codereview.chromium.org/10827146/diff/12005/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/wrench_menu_model.h (right): http://codereview.chromium.org/10827146/diff/12005/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/wrench_menu_model.h:146: content::NavigationEntry* GetActiveNavigationEntry() const; Add comment describing what this function does.
On 2012/08/03 21:53:05, dfalcantara wrote: > > 1. http://www.facebook.com correctly switches to tablet but does not switch back (on > toggling back to RDS), because it has redirected to m.cnn... as described in: > https://sites.google.com/a/google.com/clank/engineering/request-desktop-site > Not sure why it'd switch to http://m.cnn.com... do you mean it gets stuck on > m.facebook.com? Could I drop by and see this some time? > > > 2. When exiting and restating Chrome, the bool state IsUAOverridden is > remembered (per tab), but the actual UA override string is not. > > This should actually be written out; there's even two unit tests to check this. > Does ChromeOS not follow the typical session_service/tab_restore_service > pathways? > > > I can see benefits for both scenarios "per tab (all entries)" vs. "per current > entry only". I'm leaning towards the "Per Tab (for all entries)" scenario. Once > the user toggles "RTS" to on, it "sticks" for the current tab. I think this may > be more intuitive. > > FWIW, I think Bling does the former and Clank does the latter. Clank did it on > a per-navigation basis because it made sense when navigating backwards: if > you're using a per-tab setting, if you initially loaded a page with a desktop > UA, navigate somewhere, flip the flag, and navigate back to the original page, > the menu would indicate that it's using a tablet UA while showing the page > loaded with a desktop UA until the page is reloaded. It seems like it'd be more > confusing in that case. > > http://codereview.chromium.org/10827146/diff/12005/chrome/browser/ui/toolbar/... > File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): > > http://codereview.chromium.org/10827146/diff/12005/chrome/browser/ui/toolbar/... > chrome/browser/ui/toolbar/wrench_menu_model.cc:674: content::NavigationEntry* > WrenchMenuModel::GetActiveNavigationEntry() const > Style: Brace goes at the end of the previous line. > > http://codereview.chromium.org/10827146/diff/12005/chrome/browser/ui/toolbar/... > File chrome/browser/ui/toolbar/wrench_menu_model.h (right): > > http://codereview.chromium.org/10827146/diff/12005/chrome/browser/ui/toolbar/... > chrome/browser/ui/toolbar/wrench_menu_model.h:146: content::NavigationEntry* > GetActiveNavigationEntry() const; > Add comment describing what this function does. Thanks much, Dan and also for in-person conversation. I will change the behavior to the "Clank" type: Per Entry: an RTS flag is only for current entry (and future) on current tab, and not for entries in tab history. I think that's also what's Rick originally proposed.
impl. review comments; back to "RTS Flag is Per Entry", not the whole Tab history; Dan will upstream some more changes that will fix some of the behavior; Thanks for your quick and helpful reviews http://codereview.chromium.org/10827146/diff/2001/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_commands.cc (right): http://codereview.chromium.org/10827146/diff/2001/chrome/browser/ui/browser_c... chrome/browser/ui/browser_commands.cc:877: current_tab->SetUserAgentOverride(std::string()); On 2012/08/03 18:11:43, sschmitz wrote: > On 2012/08/03 15:53:06, Rick Byers wrote: > > On 2012/08/03 00:08:26, dfalcantara wrote: > > > I don't think you have to worry about un-setting the user agent override Later: After more talk with Dan, I changed it back to RTS Per Entry, i.e. not for past entries in the current tab, but only the current and further new ones. > > string > > > here, unless you want to be doubly sure that the string doesn't get used. > > > Setting the boolean to false should probably be enough. > > > > > > Also, if this is going to persist as a tab-level setting, you should iterate > > > over all of the NavigationEntrys in the NavigationController and set the > > > override flags accordingly. > > > > I'm not sure that's what we want. This is about scenarios like the following, > > right? > > 1. Go to a site > > 2. Click on a link > > 3. Turn on 'request tablet site' > > 4. Click back > > > > In that case I think we should be back in the state where the setting is off - > > just like Android. I may have over-simplified earlier saying that the setting > > should persist for the life of the tab. I meant just that when it's enabled > and > > you enter a new URL in the omnibox, it would still be enabled. I think it > would > > be confusing for the user if the state wasn't preserved with back/forward. > > Yes, good question. I can see benefits for both scenarios "per tab (all > entries)" vs. "per current entry only". I'm leaning towards the "Per Tab (for > all entries)" scenario. Once the user toggles "RTS" to on, it "sticks" for the > current tab. I think this may be more intuitive. > I was already surprised to learn that "wrench menu settings" can be for the > current tab only. I had erroneously assumed it applies for all tabs (even > windows) from "then on". Therefore, to make a "wrench menu setting" (in this > case "RTS") even more fine grained may be unintuitive for some users. http://codereview.chromium.org/10827146/diff/12005/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): http://codereview.chromium.org/10827146/diff/12005/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/wrench_menu_model.cc:674: content::NavigationEntry* WrenchMenuModel::GetActiveNavigationEntry() const On 2012/08/03 21:53:06, dfalcantara wrote: > Style: Brace goes at the end of the previous line. Done. http://codereview.chromium.org/10827146/diff/12005/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/wrench_menu_model.h (right): http://codereview.chromium.org/10827146/diff/12005/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/wrench_menu_model.h:146: content::NavigationEntry* GetActiveNavigationEntry() const; On 2012/08/03 21:53:06, dfalcantara wrote: > Add comment describing what this function does. Done.
https://chromiumcodereview.appspot.com/10827146/diff/10018/chrome/browser/ui/... File chrome/browser/ui/browser_command_controller.cc (right): https://chromiumcodereview.appspot.com/10827146/diff/10018/chrome/browser/ui/... chrome/browser/ui/browser_command_controller.cc:750: command_updater_.UpdateCommandEnabled(IDC_TOGGLE_REQUEST_TABLET_SITE, true); This shouldn't be grouped with 'show various bits of the ui'. Maybe move to 755ish with a newline above/below it. https://chromiumcodereview.appspot.com/10827146/diff/10018/chrome/browser/ui/... File chrome/browser/ui/browser_commands.cc (right): https://chromiumcodereview.appspot.com/10827146/diff/10018/chrome/browser/ui/... chrome/browser/ui/browser_commands.cc:870: { { should be on previous line. https://chromiumcodereview.appspot.com/10827146/diff/10018/chrome/browser/ui/... chrome/browser/ui/browser_commands.cc:871: // Note: The associated control is disabled when current_tab This comment is misleading so you don't do that in browser_command_controller. https://chromiumcodereview.appspot.com/10827146/diff/10018/chrome/browser/ui/... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): https://chromiumcodereview.appspot.com/10827146/diff/10018/chrome/browser/ui/... chrome/browser/ui/toolbar/wrench_menu_model.cc:367: content::NavigationEntry* entry = GetActiveNavigationEntry(); Can this functionality (and 382) be moved to browser_command_controller?
Also note that I'm not an OWNER of webkit, so make sure you have a reviewer that can cover that.
-updated based on review by sky@ -added creis@ for review of making ReloadOriginalRequestURL accessible (navigation_controller, ..._impl) per discussion with dfalcantara - added jamesr@ for review of webkit/glue/user_agent* http://codereview.chromium.org/10827146/diff/10018/chrome/browser/ui/browser_... File chrome/browser/ui/browser_command_controller.cc (right): http://codereview.chromium.org/10827146/diff/10018/chrome/browser/ui/browser_... chrome/browser/ui/browser_command_controller.cc:750: command_updater_.UpdateCommandEnabled(IDC_TOGGLE_REQUEST_TABLET_SITE, true); On 2012/08/06 21:41:06, sky wrote: > This shouldn't be grouped with 'show various bits of the ui'. Maybe move to > 755ish with a newline above/below it. Done. http://codereview.chromium.org/10827146/diff/10018/chrome/browser/ui/browser_... File chrome/browser/ui/browser_commands.cc (right): http://codereview.chromium.org/10827146/diff/10018/chrome/browser/ui/browser_... chrome/browser/ui/browser_commands.cc:870: { On 2012/08/06 21:41:06, sky wrote: > { should be on previous line. Done. http://codereview.chromium.org/10827146/diff/10018/chrome/browser/ui/browser_... chrome/browser/ui/browser_commands.cc:871: // Note: The associated control is disabled when current_tab On 2012/08/06 21:41:06, sky wrote: > This comment is misleading so you don't do that in browser_command_controller. Removed the comment. Some reorg. Done. http://codereview.chromium.org/10827146/diff/10018/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): http://codereview.chromium.org/10827146/diff/10018/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/wrench_menu_model.cc:367: content::NavigationEntry* entry = GetActiveNavigationEntry(); On 2012/08/06 21:41:06, sky wrote: > Can this functionality (and 382) be moved to browser_command_controller? I moved "382", IsCommandEnabled, to browser_command_controller.cc as you suggested. I left "363", IsCommandIdChecked, the way it is because I saw no easy way to move it and it is analogous to IDC_SHOW_BOOKMARK_BAR
Why does this feature require patching webkit/glue/user_agent.h/cc, but not the "Request desktop site" feature from Chrome/Android? Is that feature implemented in a different way? Naively, I would expect that the two should be implemented in the same way.
On 2012/08/07 02:23:55, jamesr wrote: > Why does this feature require patching webkit/glue/user_agent.h/cc, but not the > "Request desktop site" feature from Chrome/Android? Is that feature implemented > in a different way? Naively, I would expect that the two should be implemented > in the same way. The Android port does require touching this file, but the way we changed the file was too ugly to upstream. We're looking at alternative ways of accomplishing the same thing as our current hack (http://crbug.com/128570) while we upstream the other bits.
On 2012/08/07 03:42:02, dfalcantara wrote: > On 2012/08/07 02:23:55, jamesr wrote: > > Why does this feature require patching webkit/glue/user_agent.h/cc, but not > the > > "Request desktop site" feature from Chrome/Android? Is that feature > implemented > > in a different way? Naively, I would expect that the two should be > implemented > > in the same way. > > The Android port does require touching this file, but the way we changed the > file was too ugly to upstream. We're looking at alternative ways of > accomplishing the same thing as our current hack (http://crbug.com/128570) while > we upstream the other bits. Is this approach something that you would adopt for Android? I don't have strong feelings on what exactly this mechanism is, so long as we have only one version of it.
On 2012/08/07 04:35:02, jamesr wrote: > On 2012/08/07 03:42:02, dfalcantara wrote: > > On 2012/08/07 02:23:55, jamesr wrote: > > > Why does this feature require patching webkit/glue/user_agent.h/cc, but not > > the > > > "Request desktop site" feature from Chrome/Android? Is that feature > > implemented > > > in a different way? Naively, I would expect that the two should be > > implemented > > > in the same way. > > > > The Android port does require touching this file, but the way we changed the > > file was too ugly to upstream. We're looking at alternative ways of > > accomplishing the same thing as our current hack (http://crbug.com/128570) > while > > we upstream the other bits. > > Is this approach something that you would adopt for Android? I don't have > strong feelings on what exactly this mechanism is, so long as we have only one > version of it. The main problem is that creating an Android user agent (or a string for another platform) is that we can't call OS-specific functions required to build an authentic string. Searching and replacing bits of the string is probably the best option off the top of my head. If I come up with something better, I'll be sure to make it general so that ChromeOS and Android can use it when we unfork this file.
http://codereview.chromium.org/10827146/diff/5015/webkit/glue/user_agent.cc File webkit/glue/user_agent.cc (right): http://codereview.chromium.org/10827146/diff/5015/webkit/glue/user_agent.cc#n... webkit/glue/user_agent.cc:183: size_t p1 = user_agent.find("("); This seems pretty gross. If you look at what's going on here, the browser code is first getting the user agent string by calling content::GetUserAgent(), which plumbs down eventually into webkit_glue, and then calling back into webkit_glue again to rewrite part of it. I think it would be a lot cleaner if you only made one call through this stack to build the UA string you want. Take a look at UserAgentState in webkit_glue.cc - it has logic to build different UA strings for different situations. I think I'd prefer plumbing through a way to ask for a specific UA string from up in chrome/browser/ui/browser_commands.cc with just one call. You could expose different GetUserAgent() calls (GetUserAgentForTablet() perhaps) or provide a parameter to GetUserAgent indicating which UA string you want, or something of that nature. You can cache these alternate UA strings on UserAgentState like we do for user_agent_for_spoofing_hack_.
I'm ok with making ReloadOriginalRequestURL public, but I agree with James about wishing this shared more with the Android Request Desktop Site feature. We put a lot of care into making sure that behaved correctly for navigation entries (e.g., as you go back forward), and it's not clear to me whether this is sharing that logic. http://codereview.chromium.org/10827146/diff/5015/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_commands.cc (right): http://codereview.chromium.org/10827146/diff/5015/chrome/browser/ui/browser_c... chrome/browser/ui/browser_commands.cc:894: if (!current_tab->GetController().GetActiveEntry()) nit: return !!current_tab->GetController().GetActiveEntry() http://codereview.chromium.org/10827146/diff/5015/webkit/glue/user_agent.cc File webkit/glue/user_agent.cc (right): http://codereview.chromium.org/10827146/diff/5015/webkit/glue/user_agent.cc#n... webkit/glue/user_agent.cc:33: "(Linux; Android 4.0.4; Galaxy Nexus Build/IMM76B)"; Do we really want this hard coded?
On 2012/08/13 20:38:46, creis wrote: > I'm ok with making ReloadOriginalRequestURL public, but I agree with James about > wishing this shared more with the Android Request Desktop Site feature. We put > a lot of care into making sure that behaved correctly for navigation entries > (e.g., as you go back forward), and it's not clear to me whether this is sharing > that logic. I agree we should be sharing as much as is practical. > http://codereview.chromium.org/10827146/diff/5015/webkit/glue/user_agent.cc#n... > webkit/glue/user_agent.cc:33: "(Linux; Android 4.0.4; Galaxy Nexus > Build/IMM76B)"; > Do we really want this hard coded? What does Android do here? I don't see much better option myself...
http://codereview.chromium.org/10827146/diff/5015/webkit/glue/user_agent.cc File webkit/glue/user_agent.cc (right): http://codereview.chromium.org/10827146/diff/5015/webkit/glue/user_agent.cc#n... webkit/glue/user_agent.cc:33: "(Linux; Android 4.0.4; Galaxy Nexus Build/IMM76B)"; On 2012/08/13 20:38:46, creis wrote: > Do we really want this hard coded? I'm not sure if there's a good option other than setting sensible defaults. The information about the build type and OS version come directly from Android itself, which isn't available when you're running ChromeOS.
https://chromiumcodereview.appspot.com/10827146/diff/5015/webkit/glue/user_ag... File webkit/glue/user_agent.cc (right): https://chromiumcodereview.appspot.com/10827146/diff/5015/webkit/glue/user_ag... webkit/glue/user_agent.cc:33: "(Linux; Android 4.0.4; Galaxy Nexus Build/IMM76B)"; Though there is one issue with this: a Galaxy Nexus is a phone. You should stick in tablet name in there.
On 2012/08/14 01:15:16, dfalcantara wrote: > https://chromiumcodereview.appspot.com/10827146/diff/5015/webkit/glue/user_ag... > File webkit/glue/user_agent.cc (right): > > https://chromiumcodereview.appspot.com/10827146/diff/5015/webkit/glue/user_ag... > webkit/glue/user_agent.cc:33: "(Linux; Android 4.0.4; Galaxy Nexus > Build/IMM76B)"; > Though there is one issue with this: a Galaxy Nexus is a phone. You should > stick in tablet name in there. Er, I'd just wait for my CL like jamesr suggested. We're currently discussing options on what string to use when spoofing an Android user agent.
Incorporated dfalcantara@'s work to clean-up setting the spoofed user agent
string. The only real changes since the last review round (after patch 8) are:
webkit/glue/user_agent.cc
std::string BuildUserAgentOverrideForTabletSiteFromProduct(
const std::string& product) {
return BuildUserAgentFromOSAndProduct(kOsOverrideForTabletSite,
product);
}
chrome/browser/ui/browser_commands.cc
current_tab->SetUserAgentOverride(
webkit_glue::BuildUserAgentOverrideForTabletSiteFromProduct(
content::GetContentClient()->GetProduct()));
The OS part of the spoofed ua is the most simple (w/out version #): "Linux;
Android"
See https://developers.google.com/chrome/mobile/docs/user-agent:
Tablet pattern: 'Android' + 'Chrome/[.0-9]* (?!Mobile)'
http://codereview.chromium.org/10827146/diff/5015/chrome/browser/ui/browser_c...
File chrome/browser/ui/browser_commands.cc (right):
http://codereview.chromium.org/10827146/diff/5015/chrome/browser/ui/browser_c...
chrome/browser/ui/browser_commands.cc:894: if
(!current_tab->GetController().GetActiveEntry())
On 2012/08/13 20:38:46, creis wrote:
> nit: return !!current_tab->GetController().GetActiveEntry()
Done.
http://codereview.chromium.org/10827146/diff/5015/webkit/glue/user_agent.cc
File webkit/glue/user_agent.cc (right):
http://codereview.chromium.org/10827146/diff/5015/webkit/glue/user_agent.cc#n...
webkit/glue/user_agent.cc:33: "(Linux; Android 4.0.4; Galaxy Nexus
Build/IMM76B)";
On 2012/08/14 01:15:16, dfalcantara wrote:
> Though there is one issue with this: a Galaxy Nexus is a phone. You should
> stick in tablet name in there.
This has been cleaned up & shortened to "Linux; Android"
http://codereview.chromium.org/10827146/diff/5015/webkit/glue/user_agent.cc#n...
webkit/glue/user_agent.cc:183: size_t p1 = user_agent.find("(");
On 2012/08/07 19:49:03, jamesr wrote:
> This seems pretty gross.
>
> If you look at what's going on here, the browser code is first getting the
user
> agent string by calling content::GetUserAgent(), which plumbs down eventually
> into webkit_glue, and then calling back into webkit_glue again to rewrite part
> of it. I think it would be a lot cleaner if you only made one call through
this
> stack to build the UA string you want. Take a look at UserAgentState in
> webkit_glue.cc - it has logic to build different UA strings for different
> situations.
>
> I think I'd prefer plumbing through a way to ask for a specific UA string from
> up in chrome/browser/ui/browser_commands.cc with just one call. You could
> expose different GetUserAgent() calls (GetUserAgentForTablet() perhaps) or
> provide a parameter to GetUserAgent indicating which UA string you want, or
> something of that nature. You can cache these alternate UA strings on
> UserAgentState like we do for user_agent_for_spoofing_hack_.
This has been cleaned up thanks to Dan's work.
Done.
https://chromiumcodereview.appspot.com/10827146/diff/23002/chrome/browser/ui/... File chrome/browser/ui/browser_commands.cc (right): https://chromiumcodereview.appspot.com/10827146/diff/23002/chrome/browser/ui/... chrome/browser/ui/browser_commands.cc:898: return !!current_tab->GetController().GetActiveEntry(); Is this supposed to be a double !!? https://chromiumcodereview.appspot.com/10827146/diff/23002/chrome/browser/ui/... File chrome/browser/ui/browser_commands.h (right): https://chromiumcodereview.appspot.com/10827146/diff/23002/chrome/browser/ui/... chrome/browser/ui/browser_commands.h:136: bool CanRequestTabletSite(Browser* browser_); Remove _ https://chromiumcodereview.appspot.com/10827146/diff/23002/chrome/common/chro... File chrome/common/chrome_switches.cc (right): https://chromiumcodereview.appspot.com/10827146/diff/23002/chrome/common/chro... chrome/common/chrome_switches.cc:1451: "enable-request-tablet-site-even-if-non-touch-device"; // for testing Remove the "// for testing"; it's spelled out in the earlier comment. https://chromiumcodereview.appspot.com/10827146/diff/23002/webkit/glue/user_a... File webkit/glue/user_agent.cc (right): https://chromiumcodereview.appspot.com/10827146/diff/23002/webkit/glue/user_a... webkit/glue/user_agent.cc:24: const char kOsOverrideForTabletSite[] = "Linux; Android"; Change this to "Linux; Android 4.0.3" as per the email with klobag; websites might try to grep for the Android version. Not sure if this is the right place to put this, though, and I don't think we should be adding a new function for it if it's only called from one place. https://chromiumcodereview.appspot.com/10827146/diff/23002/webkit/glue/user_a... webkit/glue/user_agent.cc:188: { Brace needs to go on the previous line.
Fixes for Dan's latest review. (Thanks Dan... also for your other help) http://codereview.chromium.org/10827146/diff/23002/chrome/browser/ui/browser_... File chrome/browser/ui/browser_commands.cc (right): http://codereview.chromium.org/10827146/diff/23002/chrome/browser/ui/browser_... chrome/browser/ui/browser_commands.cc:898: return !!current_tab->GetController().GetActiveEntry(); On 2012/09/06 18:08:32, dfalcantara wrote: > Is this supposed to be a double !!? Yes, but since you ask, I changed to "... != NULL" Done http://codereview.chromium.org/10827146/diff/23002/chrome/browser/ui/browser_... File chrome/browser/ui/browser_commands.h (right): http://codereview.chromium.org/10827146/diff/23002/chrome/browser/ui/browser_... chrome/browser/ui/browser_commands.h:136: bool CanRequestTabletSite(Browser* browser_); On 2012/09/06 18:08:32, dfalcantara wrote: > Remove _ Nice catch! Done http://codereview.chromium.org/10827146/diff/23002/chrome/common/chrome_switc... File chrome/common/chrome_switches.cc (right): http://codereview.chromium.org/10827146/diff/23002/chrome/common/chrome_switc... chrome/common/chrome_switches.cc:1451: "enable-request-tablet-site-even-if-non-touch-device"; // for testing On 2012/09/06 18:08:32, dfalcantara wrote: > Remove the "// for testing"; it's spelled out in the earlier comment. Done. http://codereview.chromium.org/10827146/diff/23002/webkit/glue/user_agent.cc File webkit/glue/user_agent.cc (right): http://codereview.chromium.org/10827146/diff/23002/webkit/glue/user_agent.cc#... webkit/glue/user_agent.cc:24: const char kOsOverrideForTabletSite[] = "Linux; Android"; On 2012/09/06 18:08:32, dfalcantara wrote: > Change this to "Linux; Android 4.0.3" as per the email with klobag; websites > might try to grep for the Android version. Not sure if this is the right place > to put this, though, and I don't think we should be adding a new function for it > if it's only called from one place. Ok, thanks for your dilligence on this. Done. http://codereview.chromium.org/10827146/diff/23002/webkit/glue/user_agent.cc#... webkit/glue/user_agent.cc:188: { On 2012/09/06 18:08:32, dfalcantara wrote: > Brace needs to go on the previous line. Old habits die hard ;-) Done
http://codereview.chromium.org/10827146/diff/25006/chrome/browser/ui/browser_... File chrome/browser/ui/browser_command_controller.cc (right): http://codereview.chromium.org/10827146/diff/25006/chrome/browser/ui/browser_... chrome/browser/ui/browser_command_controller.cc:768: command_updater_.UpdateCommandEnabled(IDC_TOGGLE_REQUEST_TABLET_SITE, true); You have this set in UpdateCommandsForTabState, why do you need it here too? http://codereview.chromium.org/10827146/diff/25006/chrome/browser/ui/browser_... chrome/browser/ui/browser_command_controller.cc:916: CanRequestTabletSite(browser_)); CanRequestTabletSite only uses the active tab contents. Pass it in here (available in current_web_contents) rather than passing in browser_ and needing to get it again. http://codereview.chromium.org/10827146/diff/25006/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): http://codereview.chromium.org/10827146/diff/25006/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/wrench_menu_model.cc:371: WebContents* current_tab = chrome::GetActiveWebContents(browser_); Move this logic into a function, IsRequestingTabletSite, maybe right next to the other functions in browser_commands. http://codereview.chromium.org/10827146/diff/25006/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/wrench_menu_model.cc:550: AddItemWithStringId(IDC_OPTIONS, IDS_SETTINGS);\ WHy the \ ? http://codereview.chromium.org/10827146/diff/25006/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/wrench_menu_model.cc:553: // Note: on Chromebooks with default to "Desktop Site" even for Again, how come we don't have one in about:flags ? http://codereview.chromium.org/10827146/diff/25006/chrome/common/chrome_switc... File chrome/common/chrome_switches.cc (right): http://codereview.chromium.org/10827146/diff/25006/chrome/common/chrome_switc... chrome/common/chrome_switches.cc:1449: const char kEnableRequestTabletSite[] = "enable-request-tablet-site"; The bug makes it sounds like this is experimental, shouldn't we only have one setting and you can enable it in about:flags? http://codereview.chromium.org/10827146/diff/25006/content/browser/web_conten... File content/browser/web_contents/navigation_controller_impl.h (right): http://codereview.chromium.org/10827146/diff/25006/content/browser/web_conten... content/browser/web_contents/navigation_controller_impl.h:81: virtual void ReloadOriginalRequestURL(bool check_for_repost) OVERRIDE; Move the implementation to match new position in header.
fixes based on review by sky@ (thanks!) Patch 13 has a few rebasing changes http://codereview.chromium.org/10827146/diff/25006/chrome/browser/ui/browser_... File chrome/browser/ui/browser_command_controller.cc (right): http://codereview.chromium.org/10827146/diff/25006/chrome/browser/ui/browser_... chrome/browser/ui/browser_command_controller.cc:768: command_updater_.UpdateCommandEnabled(IDC_TOGGLE_REQUEST_TABLET_SITE, true); On 2012/09/07 18:05:30, sky wrote: > You have this set in UpdateCommandsForTabState, why do you need it here too? Removed it. Done. http://codereview.chromium.org/10827146/diff/25006/chrome/browser/ui/browser_... chrome/browser/ui/browser_command_controller.cc:916: CanRequestTabletSite(browser_)); On 2012/09/07 18:05:30, sky wrote: > CanRequestTabletSite only uses the active tab contents. Pass it in here > (available in current_web_contents) rather than passing in browser_ and needing > to get it again. Done. http://codereview.chromium.org/10827146/diff/25006/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): http://codereview.chromium.org/10827146/diff/25006/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/wrench_menu_model.cc:371: WebContents* current_tab = chrome::GetActiveWebContents(browser_); On 2012/09/07 18:05:30, sky wrote: > Move this logic into a function, IsRequestingTabletSite, maybe right next to the > other functions in browser_commands. Done. http://codereview.chromium.org/10827146/diff/25006/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/wrench_menu_model.cc:550: AddItemWithStringId(IDC_OPTIONS, IDS_SETTINGS);\ On 2012/09/07 18:05:30, sky wrote: > WHy the \ ? Oops, removed. Done. http://codereview.chromium.org/10827146/diff/25006/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/wrench_menu_model.cc:553: // Note: on Chromebooks with default to "Desktop Site" even for On 2012/09/07 18:05:30, sky wrote: > Again, how come we don't have one in about:flags ? Removed one flag, removed the "and is-touch-device" requirement, added remaining flag to about:flags. http://codereview.chromium.org/10827146/diff/25006/chrome/common/chrome_switc... File chrome/common/chrome_switches.cc (right): http://codereview.chromium.org/10827146/diff/25006/chrome/common/chrome_switc... chrome/common/chrome_switches.cc:1449: const char kEnableRequestTabletSite[] = "enable-request-tablet-site"; On 2012/09/07 18:05:30, sky wrote: > The bug makes it sounds like this is experimental, shouldn't we only have one > setting and you can enable it in about:flags? Done. http://codereview.chromium.org/10827146/diff/25006/content/browser/web_conten... File content/browser/web_contents/navigation_controller_impl.h (right): http://codereview.chromium.org/10827146/diff/25006/content/browser/web_conten... content/browser/web_contents/navigation_controller_impl.h:81: virtual void ReloadOriginalRequestURL(bool check_for_repost) OVERRIDE; On 2012/09/07 18:05:30, sky wrote: > Move the implementation to match new position in header. Done.
http://codereview.chromium.org/10827146/diff/25006/content/browser/web_conten... File content/browser/web_contents/navigation_controller_impl.h (right): http://codereview.chromium.org/10827146/diff/25006/content/browser/web_conten... content/browser/web_contents/navigation_controller_impl.h:81: virtual void ReloadOriginalRequestURL(bool check_for_repost) OVERRIDE; On 2012/09/07 22:33:09, sschmitz wrote: > On 2012/09/07 18:05:30, sky wrote: > > Move the implementation to match new position in header. > > Done. I don't see this in the latest patch. http://codereview.chromium.org/10827146/diff/26007/chrome/app/generated_resou... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/10827146/diff/26007/chrome/app/generated_resou... chrome/app/generated_resources.grd:1306: + <message name="IDS_TOGGLE_REQUEST_TABLET_SITE" desc="The toggle to request tablet site on a touch dev."> Since this is only for chromeos you don't need both variants. http://codereview.chromium.org/10827146/diff/26007/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): http://codereview.chromium.org/10827146/diff/26007/chrome/browser/about_flags... chrome/browser/about_flags.cc:936: IDS_ENABLE_REQUEST_TABLET_SITE_NAME, Your name should match that of others in this file, IDS_FLAGS_... http://codereview.chromium.org/10827146/diff/26007/chrome/browser/about_flags... chrome/browser/about_flags.cc:938: kOsCrOS | kOsLinux, Remove kOsLinux here since you're ifdef is for chromeos.
On Fri, Sep 7, 2012 at 3:54 PM, <sky@chromium.org> wrote: > > http://codereview.chromium.**org/10827146/diff/25006/** > content/browser/web_contents/**navigation_controller_impl.h<http://codereview.chromium.org/10827146/diff/25006/content/browser/web_contents/navigation_controller_impl.h> > File content/browser/web_contents/**navigation_controller_impl.h (right): > > http://codereview.chromium.**org/10827146/diff/25006/** > content/browser/web_contents/**navigation_controller_impl.h#**newcode81<http://codereview.chromium.org/10827146/diff/25006/content/browser/web_contents/navigation_controller_impl.h#newcode81> > content/browser/web_contents/**navigation_controller_impl.h:**81: virtual > void ReloadOriginalRequestURL(bool check_for_repost) OVERRIDE; > On 2012/09/07 22:33:09, sschmitz wrote: > >> On 2012/09/07 18:05:30, sky wrote: >> > Move the implementation to match new position in header. >> > > Done. >> > > I don't see this in the latest patch. > void ReloadOriginalRequestURL(bool check_for_repost) is already in the correct position in content/browser/web_contents/navigation_controller_impl.cc > > http://codereview.chromium.**org/10827146/diff/26007/** > chrome/app/generated_**resources.grd<http://codereview.chromium.org/10827146/diff/26007/chrome/app/generated_resources.grd> > File chrome/app/generated_**resources.grd (right): > > http://codereview.chromium.**org/10827146/diff/26007/** > chrome/app/generated_**resources.grd#newcode1306<http://codereview.chromium.org/10827146/diff/26007/chrome/app/generated_resources.grd#newcode1306> > chrome/app/generated_**resources.grd:1306: + <message > name="IDS_TOGGLE_REQUEST_**TABLET_SITE" desc="The toggle to request tablet > site on a touch dev."> > Since this is only for chromeos you don't need both variants. > > http://codereview.chromium.**org/10827146/diff/26007/** > chrome/browser/about_flags.cc<http://codereview.chromium.org/10827146/diff/26007/chrome/browser/about_flags.cc> > File chrome/browser/about_flags.cc (right): > > http://codereview.chromium.**org/10827146/diff/26007/** > chrome/browser/about_flags.cc#**newcode936<http://codereview.chromium.org/10827146/diff/26007/chrome/browser/about_flags.cc#newcode936> > chrome/browser/about_flags.cc:**936: IDS_ENABLE_REQUEST_TABLET_** > SITE_NAME, > Your name should match that of others in this file, IDS_FLAGS_... > > http://codereview.chromium.**org/10827146/diff/26007/** > chrome/browser/about_flags.cc#**newcode938<http://codereview.chromium.org/10827146/diff/26007/chrome/browser/about_flags.cc#newcode938> > chrome/browser/about_flags.cc:**938: kOsCrOS | kOsLinux, > Remove kOsLinux here since you're ifdef is for chromeos. > > http://codereview.chromium.**org/10827146/<http://codereview.chromium.org/108... >
http://codereview.chromium.org/10827146/diff/26007/chrome/app/generated_resou... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/10827146/diff/26007/chrome/app/generated_resou... chrome/app/generated_resources.grd:1306: + <message name="IDS_TOGGLE_REQUEST_TABLET_SITE" desc="The toggle to request tablet site on a touch dev."> On 2012/09/07 22:54:16, sky wrote: > Since this is only for chromeos you don't need both variants. Done. http://codereview.chromium.org/10827146/diff/26007/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): http://codereview.chromium.org/10827146/diff/26007/chrome/browser/about_flags... chrome/browser/about_flags.cc:936: IDS_ENABLE_REQUEST_TABLET_SITE_NAME, On 2012/09/07 22:54:16, sky wrote: > Your name should match that of others in this file, IDS_FLAGS_... Done. http://codereview.chromium.org/10827146/diff/26007/chrome/browser/about_flags... chrome/browser/about_flags.cc:938: kOsCrOS | kOsLinux, On 2012/09/07 22:54:16, sky wrote: > Remove kOsLinux here since you're ifdef is for chromeos. Done.
LGTM
http://codereview.chromium.org/10827146/diff/19006/webkit/glue/user_agent.h File webkit/glue/user_agent.h (right): http://codereview.chromium.org/10827146/diff/19006/webkit/glue/user_agent.h#n... webkit/glue/user_agent.h:33: std::string BuildUserAgentOverrideForTabletSiteFromProduct( I don't think you really want a separate entry point here. There's only one caller and the "type" of tablet override is specific to that caller. I would suggest not patching webkit/glue/ at all and have the caller specify whatever magical string it wants in a call to BuildUserAgentFromOSAndProduct()
Thanks James. I made the change. webkit/gue/user_agent.h&cc are not used from master.... thanks to Dan's great work. http://codereview.chromium.org/10827146/diff/19006/webkit/glue/user_agent.h File webkit/glue/user_agent.h (right): http://codereview.chromium.org/10827146/diff/19006/webkit/glue/user_agent.h#n... webkit/glue/user_agent.h:33: std::string BuildUserAgentOverrideForTabletSiteFromProduct( On 2012/09/07 23:58:30, jamesr wrote: > I don't think you really want a separate entry point here. There's only one > caller and the "type" of tablet override is specific to that caller. I would > suggest not patching webkit/glue/ at all and have the caller specify whatever > magical string it wants in a call to BuildUserAgentFromOSAndProduct() I agree. I removed this function from user_agent.h/cc. They are no longer part of this CL as the master version is what we now use. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/10827146/23025
Presubmit check for 10827146-23025 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
content/public/browser
Presubmit checks took 1.7s to calculate.
On 2012/09/10 14:34:16, I haz the power (commit-bot) wrote: > Presubmit check for 10827146-23025 failed and returned exit status 1. > > > Running presubmit commit checks ... > > ** Presubmit ERRORS ** > Missing LGTM from an OWNER for files in these directories: > content/public/browser > > Presubmit checks took 1.7s to calculate. adding joi@ as owner of content/public for change: Moving from private to public: void ReloadOriginalRequestURL(bool check_for_repost) in content/public/browser/navigation_controller.h See comment by creis@
Added joi@ as owner of content/public for content/public/browser/navigation_controller.h (Making a mem fct public)
LGTM for content/public.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/10827146/23025
Try job failure for 10827146-23025 (retry) on android for steps "compile, build" (clobber build). It's a second try, previously, steps "compile, build" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/10827146/26015
Change committed as 155872 |
