Fix user agent override restore pipeline

Fix user agent override restoration pipeline
    
There were some disconnects in the pipeline that prevented the user agent override
string from being saved and restored from the temporary tab structions in sessions/,
but couldn't be tested on Android because the pathways weren't being exercised in
the same way.  This CL finishes hooking it up for ChromeOS (and presumably the other
platforms).
    
BUG=140600
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152207

[gone, 2012-08-06 20:09:39]

sky:
Added you as reviewer since you helped with my previous session restoring CL.  I
think you're the right guy to ping about this, but please redirect if I'm
incorrect.

[gone, 2012-08-08 21:46:56]

Ping?

[sky, 2012-08-08 22:16:07]

Add tests to cover this.

[gone, 2012-08-13 21:54:00]

On 2012/08/08 22:16:07, sky wrote:
> Add tests to cover this.

Changed 3 unit tests to cover two pathways.

[sky, 2012-08-14 00:05:46]

LGTM with the following changes.

https://chromiumcodereview.appspot.com/10827168/diff/10012/chrome/browser/ses...
File chrome/browser/sessions/session_restore_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10827168/diff/10012/chrome/browser/ses...
chrome/browser/sessions/session_restore_browsertest.cc:411:
ASSERT_EQ(web_contents->GetUserAgentOverride(), tab.user_agent_override);
flip the arguments here, 426 and 437.

https://chromiumcodereview.appspot.com/10827168/diff/10012/chrome/browser/ses...
File chrome/browser/sessions/tab_restore_service_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10827168/diff/10012/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_browsertest.cc:167: EXPECT_TRUE(""
== tab->user_agent_override);
EXPECT_EQ

https://chromiumcodereview.appspot.com/10827168/diff/10012/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_browsertest.cc:190:
EXPECT_TRUE(user_agent_override_ == tab->user_agent_override);
EXPECT_EQ

[gone, 2012-08-14 01:03:22]

Done, thanks!

https://chromiumcodereview.appspot.com/10827168/diff/10012/chrome/browser/ses...
File chrome/browser/sessions/session_restore_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10827168/diff/10012/chrome/browser/ses...
chrome/browser/sessions/session_restore_browsertest.cc:411:
ASSERT_EQ(web_contents->GetUserAgentOverride(), tab.user_agent_override);
On 2012/08/14 00:05:46, sky wrote:
> flip the arguments here, 426 and 437.

Done.

https://chromiumcodereview.appspot.com/10827168/diff/10012/chrome/browser/ses...
File chrome/browser/sessions/tab_restore_service_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10827168/diff/10012/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_browsertest.cc:167: EXPECT_TRUE(""
== tab->user_agent_override);
On 2012/08/14 00:05:46, sky wrote:
> EXPECT_EQ

Done.

https://chromiumcodereview.appspot.com/10827168/diff/10012/chrome/browser/ses...
chrome/browser/sessions/tab_restore_service_browsertest.cc:190:
EXPECT_TRUE(user_agent_override_ == tab->user_agent_override);
On 2012/08/14 00:05:46, sky wrote:
> EXPECT_EQ

Done.

[gone, 2012-08-15 00:22:09]

+creis

Unfortunately, I found another hole in the pipeline while testing it out.  The
hole existed because a user agent override change wasn't triggering
SessionService::SetTabUserAgentOverride(), meaning that it was never being set
for the SessionTab.  The fix required adding a new Notification type to content
that gets fired whenever the user agent is set.

I've added a new unit test to catch it in session_restore_browsertest.cc.

Please take another look when you've got the chance.  Sorry I didn't catch it
sooner!

[sschmitz, 2012-08-15 15:33:01]

Dan,

Can you think of a test case (for my desktop) that would fail without
your latest update but succeed with it?

If so I could run that before and after applying your latest update.

Stefan

On Tue, Aug 14, 2012 at 5:22 PM,  <dfalcantara@chromium.org> wrote:
>
> +creis
>
> Unfortunately, I found another hole in the pipeline while testing it out.
> The
> hole existed because a user agent override change wasn't triggering
> SessionService::SetTabUserAgentOverride(), meaning that it was never being
> set
> for the SessionTab.  The fix required adding a new Notification type to
> content
> that gets fired whenever the user agent is set.
>
> I've added a new unit test to catch it in session_restore_browsertest.cc.
>
> Please take another look when you've got the chance.  Sorry I didn't catch
> it
> sooner!
>
> https://chromiumcodereview.appspot.com/10827168/

[gone, 2012-08-15 15:38:18]

> Dan,
> 
> Can you think of a test case (for my desktop) that would fail without
> your latest update but succeed with it?
> 
> If so I could run that before and after applying your latest update.
> 
> Stefan

I'm pretty sure the tests I added will fail without the rest of the CL:
SessionRestoreTest.RestoreForeignTab
SessionRestoreTest.RestoreForeignSession
SessionRestoreTest.PersistAndRestoreUserAgentOverride
TabRestoreServiceTest.Basic

[Charlie Reis, 2012-08-15 17:24:22]

LGTM.  Adding John for content/public/browser/notification_types.h, but I think
it's ok in this case.

https://chromiumcodereview.appspot.com/10827168/diff/3021/chrome/browser/sess...
File chrome/browser/sessions/session_restore_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10827168/diff/3021/chrome/browser/sess...
chrome/browser/sessions/session_restore_browsertest.cc:395:
tab.user_agent_override = "user_agent_override";
Can you make this a realistic value?  Tests also serve as useful documentation
to help other developers understand what the expected behavior is (and it's not
clear if we actually expect this to be a user agent or some value indicating an
override is necessary).

https://chromiumcodereview.appspot.com/10827168/diff/3021/chrome/browser/ui/b...
File chrome/browser/ui/browser_tabrestore.h (right):

https://chromiumcodereview.appspot.com/10827168/diff/3021/chrome/browser/ui/b...
chrome/browser/ui/browser_tabrestore.h:34: // |navigations| are from the
previous session.
Please mention what |user_agent_override| is for.

https://chromiumcodereview.appspot.com/10827168/diff/3021/content/public/brow...
File content/public/browser/notification_types.h (right):

https://chromiumcodereview.appspot.com/10827168/diff/3021/content/public/brow...
content/public/browser/notification_types.h:326:
NOTIFICATION_WEB_CONTENTS_USER_AGENT_OVERRIDE_CHANGED,
John is typically against adding new notifications, but given that
SessionService appears to depend on them, this seems ok to me.

[gone, 2012-08-15 20:15:43]

Nits addressed, thanks!  Will wait for jam to weigh in.

https://chromiumcodereview.appspot.com/10827168/diff/3021/chrome/browser/sess...
File chrome/browser/sessions/session_restore_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10827168/diff/3021/chrome/browser/sess...
chrome/browser/sessions/session_restore_browsertest.cc:395:
tab.user_agent_override = "user_agent_override";
On 2012/08/15 17:24:23, creis wrote:
> Can you make this a realistic value?  Tests also serve as useful documentation
> to help other developers understand what the expected behavior is (and it's
not
> clear if we actually expect this to be a user agent or some value indicating
an
> override is necessary).

Stuck in the one we're currently using to spoof a desktop linux build.

https://chromiumcodereview.appspot.com/10827168/diff/3021/chrome/browser/ui/b...
File chrome/browser/ui/browser_tabrestore.h (right):

https://chromiumcodereview.appspot.com/10827168/diff/3021/chrome/browser/ui/b...
chrome/browser/ui/browser_tabrestore.h:34: // |navigations| are from the
previous session.
On 2012/08/15 17:24:23, creis wrote:
> Please mention what |user_agent_override| is for.

Done.

[jam, 2012-08-15 23:36:17]

On 2012/08/15 17:24:22, creis wrote:
> LGTM.  Adding John for content/public/browser/notification_types.h, but I
think
> it's ok in this case.

I dislike notifications very much and I've been trying to stop as many more
getting added as possible :) they're often misused, and create a parallel api to
the content api in this example. i.e. random code in chrome gets notified from
random code in content. existing interfaces can already allow this, i.e.
WebContentsImpl can call a new method on WebContentsObserver, which TabContents
observes and then it can call SessionService for the relevant profile.

[gone, 2012-08-16 21:17:15]

> I dislike notifications very much and I've been trying to stop as many more
> getting added as possible :) they're often misused, and create a parallel api
to
> the content api in this example. i.e. random code in chrome gets notified from
> random code in content. existing interfaces can already allow this, i.e.
> WebContentsImpl can call a new method on WebContentsObserver, which
TabContents
> observes and then it can call SessionService for the relevant profile.

Thanks for the heads up on that pathway!  The only reason I used the
notification was because SessionService already seemed to use them for its other
triggers; I've gone ahead and swapped over to using observers and am running the
unit tests again.  Please take a look.

[jam, 2012-08-17 16:02:09]

lgtm with nits, thanks for updating

yes definitely it was the norm previously to just add notifications everywhere.
i'm trying to cut this back, at least for code in content (I would love to
remove all the ones in content)

http://codereview.chromium.org/10827168/diff/2010/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/10827168/diff/2010/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents.h:258: 
nit: no blank line

http://codereview.chromium.org/10827168/diff/2010/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

http://codereview.chromium.org/10827168/diff/2010/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:797: // Alert the observer
that the user agent was changed.
nit: this comment is gratuitious, please remove

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem...

http://codereview.chromium.org/10827168/diff/2010/content/public/browser/web_...
File content/public/browser/web_contents_observer.h (right):

http://codereview.chromium.org/10827168/diff/2010/content/public/browser/web_...
content/public/browser/web_contents_observer.h:130: virtual void
SetUserAgentOverride(const std::string& user_agent) {}
nit: name this UserAgentOverrideSet or UserAgentOverridden, so that it's clear
that this is a notification, not asking the observer to do something

[gone, 2012-08-17 18:39:55]

Nits fixed, thanks!  If there aren't any other comments, I'll send this to the
commit bot after lunch.

https://chromiumcodereview.appspot.com/10827168/diff/2010/chrome/browser/ui/t...
File chrome/browser/ui/tab_contents/tab_contents.h (right):

https://chromiumcodereview.appspot.com/10827168/diff/2010/chrome/browser/ui/t...
chrome/browser/ui/tab_contents/tab_contents.h:258: 
On 2012/08/17 16:02:09, John Abd-El-Malek wrote:
> nit: no blank line

Done.

https://chromiumcodereview.appspot.com/10827168/diff/2010/content/browser/web...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/10827168/diff/2010/content/browser/web...
content/browser/web_contents/web_contents_impl.cc:797: // Alert the observer
that the user agent was changed.
On 2012/08/17 16:02:09, John Abd-El-Malek wrote:
> nit: this comment is gratuitious, please remove
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem...

Done.

https://chromiumcodereview.appspot.com/10827168/diff/2010/content/public/brow...
File content/public/browser/web_contents_observer.h (right):

https://chromiumcodereview.appspot.com/10827168/diff/2010/content/public/brow...
content/public/browser/web_contents_observer.h:130: virtual void
SetUserAgentOverride(const std::string& user_agent) {}
On 2012/08/17 16:02:09, John Abd-El-Malek wrote:
> nit: name this UserAgentOverrideSet or UserAgentOverridden, so that it's clear
> that this is a notification, not asking the observer to do something

Done.

[commit-bot: I haz the power, 2012-08-17 20:31:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/10827168/15008

[commit-bot: I haz the power, 2012-08-18 01:47:54]

Change committed as 152207

[Avi (use Gerrit), 2012-08-29 21:01:22]

https://chromiumcodereview.appspot.com/10827168/diff/15008/chrome/browser/ui/...
File chrome/browser/ui/tab_contents/tab_contents.cc (right):

https://chromiumcodereview.appspot.com/10827168/diff/15008/chrome/browser/ui/...
chrome/browser/ui/tab_contents/tab_contents.cc:266: #endif
NO.

TabContents is not the place for random overrides of stuff like this. Keep
functionality belonging to various parts of the system, like the session
service, in session service code.

[gone, 2012-08-29 21:10:01]

https://chromiumcodereview.appspot.com/10827168/diff/15008/chrome/browser/ui/...
File chrome/browser/ui/tab_contents/tab_contents.cc (right):

https://chromiumcodereview.appspot.com/10827168/diff/15008/chrome/browser/ui/...
chrome/browser/ui/tab_contents/tab_contents.cc:266: #endif
On 2012/08/29 21:01:22, Avi wrote:
> NO.
> 
> TabContents is not the place for random overrides of stuff like this. Keep
> functionality belonging to various parts of the system, like the session
> service, in session service code.

Er, alright.  How should this change then?  I moved code here after jam raised
concerns about using notifiers instead of observers.

[Avi (use Gerrit), 2012-08-29 21:19:16]

I'm fixing. How does changing RestoreTabHelper to SessionTabHelper and adding
this to all the stuff that it watches?

[gone, 2012-08-29 21:26:02]

On 2012/08/29 21:19:16, Avi wrote:
> I'm fixing. How does changing RestoreTabHelper to SessionTabHelper and adding
> this to all the stuff that it watches?

Sounds good to me.