Bugfix for 2932

Fix for bug 2932: Chrome should not trigger page reloading when turn off auto detect

In order to fix the bug I added new interfaces to allow resetting the override encoding (including a new render message).

The new logic is as follows:

-) If the user turns auto-detect OFF, nothing happens (as requested in the bug description)
-) If the user turns auto-detect ON, then the page is reloaded with an empty override encoding

I.e., turning auto-detect on resets a previous override setting. 
The reverse is not true, however: specifiying a new override setting will not turn auto-detect off.


BUG=2932
TEST=do the following steps, using the test file (encoding-gb18030.htm) attached to comment 14 of the original bug http://crbug.com/2932 :

1.) load the file while auto-detect is turned off
    -> garbage is displayed
2.) turn auto-detect on
    -> proper Chinese text should appear
    -> in the encoding menu, Chinese encoding should be highlighted
3.) turn auto-detect off
    -> proper Chinese text should remain
    -> in the encoding menu, Chinese encoding should remain highlighted
4.) choose any other encoding
    -> garbage is again displayed (in the new encoding)
5.) turn auto-detect on
    -> proper Chinese text should again appear
    -> in the encoding menu, Chinese should again be highlighted

[Roland, 2009-08-24 11:05:30]

Hi, I submitted a fix for bug 2932 ("Chrome should not trigger page reloading
when turn off auto detect"). I hope you guys are the right ones to review it.

Cheers,

Roland

[jungshik at Google, 2009-08-25 17:15:35]

Thank you for the CL. 

From a cursory look, it's reasonable, but I have to refresh my memory what
Webkit DocumentLoader/FrameLoader do with an empty override encoding. It's a bit
'treacherous' out there :-). I also want to test it myself with this CL applied.


In addition, it'd be better to add something to TEST (put up a web page with no
encoding specified somewhere - I can put one up at my personal web server if you
want - and describe what to test). 

Even better would be to add a UI test. 

On 2009/08/24 11:05:30, Roland wrote:
> Hi, I submitted a fix for bug 2932 ("Chrome should not trigger page reloading
> when turn off auto detect"). I hope you guys are the right ones to review it.
> 
> Cheers,
> 
> Roland

[Roland, 2009-08-26 07:14:25]

Thanks for reviewing my patch!

I tested it mostly with the file provided in the original bug desription
(http://code.google.com/p/chromium/issues/detail?id=2932).

In addition to the steps detailed therein I also implemented it such that an
override encoding is cleared if the user (re-)enables auto-detect. 

In summary, I guess the following steps should be used to test the
functionality:

1.) load the file while auto-detect is turned off
    -> garbage is displayed
2.) turn auto-detect on
    -> proper Chinese text should appear
    -> in the encoding menu, Chinese encoding should be highlighted
3.) turn auto-detect off
    -> proper Chinese text should remain
    -> in the encoding menu, Chinese encoding should remain highlighted
4.) choose any other encoding
    -> garbage is again displayed (in the new encoding)
5.) turn auto-detect on
    -> proper Chinese text should again appear
    -> in the encoding menu, Chinese should again be highlighted

I'm not entirely sure what the best way would be to encode all these steps
within a UI test, though.

[jungshik at Google, 2009-08-26 16:30:29]

Thanks a lot for the CL and the explanation. You seems to have done a rather
thorough testing. 

On 2009/08/26 07:14:25, Roland wrote:
> Thanks for reviewing my patch!
> 
> I tested it mostly with the file provided in the original bug desription
> (http://code.google.com/p/chromium/issues/detail?id=2932).
> 
> In addition to the steps detailed therein I also implemented it such that an
> override encoding is cleared if the user (re-)enables auto-detect.

Yeah, that's a good idea !
 
> 
> In summary, I guess the following steps should be used to test the
> functionality:
> 
> 1.) load the file while auto-detect is turned off
>     -> garbage is displayed
> 2.) turn auto-detect on
>     -> proper Chinese text should appear
>     -> in the encoding menu, Chinese encoding should be highlighted
> 3.) turn auto-detect off
>     -> proper Chinese text should remain
>     -> in the encoding menu, Chinese encoding should remain highlighted
> 4.) choose any other encoding
>     -> garbage is again displayed (in the new encoding)
> 5.) turn auto-detect on
>     -> proper Chinese text should again appear
>     -> in the encoding menu, Chinese should again be highlighted
> 

Thanks a lot for the description. Can you add this TEST description to your CL
description like this?

TEST=Take the following steps to test.
0. Download the file ( encoding-gb18030.htm ) attached to the bug
(http://crbug.com/2932) and save it to your local disk
1. load the .... 

..... snip 
5. turn auto-detect on ....

Having TEST=.. line in the CL description is to enable other people (especially
QA) to verify the fix. It also helps others in the future. They don't have to
come here, but can just read the SVN log (and it's also shown in the build bot).


> I'm not entirely sure what the best way would be to encode all these steps
> within a UI test, though.

We can add the UI test in a separate CL. 

Anyway, let me have another look before giving you go-ahead. I'll get back to
you this afternoon.

[jungshik at Google, 2009-09-04 21:27:03]

On 2009/08/26 16:30:29, Jungshik Shin wrote:
> Thanks a lot for the CL and the explanation. You seems to have done a rather
> thorough testing. 

Sorry that I didn't come back earlier. I did apply your patch to my local trees
on Mac, Linux and Windows. On Mac/Linux, it worked as expected, but on Windows I
have a mysterious crash on start-up (I tried to debug, but the crash happens in
a place where it appears to have nothing to do with this change. With your patch
removed, it's ok ). It might be due to my Windows tree being corrupt somehow(?).


Have you tested this on Windows as well?  

BTW, I'm sorry again that I didn't come back to you sooner. There's a patch that
is about to be landed to use std::string for encoding names. (Tony is about to
land it). Once it's landed, you have to update your CL.

[Roland, 2009-09-07 08:31:11]

Strange indeed. Sounds like a linker error to me (had some of those recently...
:p) <obligatory>Did you try a clobber build?</obligatory>.

FWIW, Yes, I developed and tested the patch under Windows.

- Roland

On 2009/09/04 21:27:03, Jungshik Shin wrote:
> Sorry that I didn't come back earlier. I did apply your patch to my local
trees
> on Mac, Linux and Windows. On Mac/Linux, it worked as expected, but on Windows
I
> have a mysterious crash on start-up (I tried to debug, but the crash happens
in
> a place where it appears to have nothing to do with this change. With your
patch
> removed, it's ok ). It might be due to my Windows tree being corrupt
somehow(?).
> 
> 
> Have you tested this on Windows as well?  
> 
> BTW, I'm sorry again that I didn't come back to you sooner. There's a patch
that
> is about to be landed to use std::string for encoding names. (Tony is about to
> land it). Once it's landed, you have to update your CL.

[Roland, 2009-09-08 01:25:01]

Updated the patch set after merging for the changes mentioned below.
(Tested under Windows)


> On 2009/09/04 21:27:03, Jungshik Shin wrote:
> > BTW, I'm sorry again that I didn't come back to you sooner. There's a patch
> that
> > is about to be landed to use std::string for encoding names. (Tony is about
to
> > land it). Once it's landed, you have to update your CL.

[jungshik at Google, 2009-09-08 17:29:12]

With the comments below addressed, it looks good. 

If others are ok, I'll land the patch (or can you land yourself, Roland?).

http://codereview.chromium.org/173265/diff/4001/4004
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/173265/diff/4001/4004#newcode1012
Line 1012: if (encoding_auto_detect_.GetValue()) {
Can you add a comment explaining what you do when it's turned on and turned off
(of course, for the latter, you do nothing)?

http://codereview.chromium.org/173265/diff/4001/4002
File webkit/glue/webview_impl.cc (right):

http://codereview.chromium.org/173265/diff/4001/4002#newcode1409
Line 1409: // TODO(brettw) use std::string for encoding names.
This TODO comment is not applicable any more. Pls, remove it.

[Roland, 2009-09-09 01:55:27]

On 2009/09/08 17:29:12, Jungshik Shin wrote:
> With the comments below addressed, it looks good. 

Uploaded a new patch set with the comments added/changed (no functional change).

> If others are ok, I'll land the patch (or can you land yourself, Roland?).

Not yet, working on it... ^_^;

[jungshik at Google, 2009-09-09 18:45:20]

Brett and Johny, do you have any comment or concern? If not, I'll land this CL. 

On 2009/09/09 01:55:27, Roland wrote:
> On 2009/09/08 17:29:12, Jungshik Shin wrote:
> > With the comments below addressed, it looks good. 
> 
> Uploaded a new patch set with the comments added/changed (no functional
change).
> 
> > If others are ok, I'll land the patch (or can you land yourself, Roland?).
> 
> Not yet, working on it... ^_^;

[Johnny(Jianning) Ding, 2009-09-10 01:52:05]

Hi Roland, Jungshik

Sorry for being late response. Thanks Roland to fix this problem.
I like this patch, but I think we could make the change more simple.

Since the flag of using auto detection is controlled on Chrome side and the
value will sync to render engine each time it gets changed (see the message:
ViewMsg_UpdateWebPreferences). We do not need to send the reset-encoding message
to render engine. I think a very simple change on browser.cc like the following
code is enough.
void Browser::ToggleEncodingAutoDetect() {
  ...
  if (encoding_auto_detect_.GetValue())
    Reload();
  ...
}

I have tested it, it works well without changing current logic.
What are your opinions?

Thanks again!


On 2009/09/09 18:45:20, Jungshik Shin wrote:
> Brett and Johny, do you have any comment or concern? If not, I'll land this
CL. 
> 
> On 2009/09/09 01:55:27, Roland wrote:
> > On 2009/09/08 17:29:12, Jungshik Shin wrote:
> > > With the comments below addressed, it looks good. 
> > 
> > Uploaded a new patch set with the comments added/changed (no functional
> change).
> > 
> > > If others are ok, I'll land the patch (or can you land yourself, Roland?).
> > 
> > Not yet, working on it... ^_^;

[Roland, 2009-09-10 05:38:56]

Hi Johnny,

implementing it the way you described was my first attempt at implementing this,
but it doesn't seem to clear the override encoding if it's set (at least not for
me... @_@?)

E.g., if I choose 'Baltic' in step 4 in my test list, step 5 will then fail:
it's still 'Baltic' after switching on 'auto-detect', and it still displays
garbage. 


On 2009/09/10 01:52:05, Johnny(Jianning) Ding wrote:
> Hi Roland, Jungshik
> 
> Sorry for being late response. Thanks Roland to fix this problem.
> I like this patch, but I think we could make the change more simple.
> 
> Since the flag of using auto detection is controlled on Chrome side and the
> value will sync to render engine each time it gets changed (see the message:
> ViewMsg_UpdateWebPreferences). We do not need to send the reset-encoding
message
> to render engine. I think a very simple change on browser.cc like the
following
> code is enough.
> void Browser::ToggleEncodingAutoDetect() {
>   ...
>   if (encoding_auto_detect_.GetValue())
>     Reload();
>   ...
> }
> 
> I have tested it, it works well without changing current logic.
> What are your opinions?
> 
> Thanks again!
> 
> 
> On 2009/09/09 18:45:20, Jungshik Shin wrote:
> > Brett and Johny, do you have any comment or concern? If not, I'll land this
> CL. 
> > 
> > On 2009/09/09 01:55:27, Roland wrote:
> > > On 2009/09/08 17:29:12, Jungshik Shin wrote:
> > > > With the comments below addressed, it looks good. 
> > > 
> > > Uploaded a new patch set with the comments added/changed (no functional
> > change).
> > > 
> > > > If others are ok, I'll land the patch (or can you land yourself,
Roland?).
> > > 
> > > Not yet, working on it... ^_^;

[Johnny(Jianning) Ding, 2009-09-10 08:13:03]

Hi Roland,

Thanks for your detail explanation.
According to your comments, the behavior you described is when enabling encoding
auto-detection, the overridden encoding (if user overrided the encoding before)
should be reset (to empty). In the other words, enabling encoding auto-detection
will override the page encoding to default encoding of current locale.

I have two comments about your logic for reference,
1) Is that right to reset the user chosen encoding when enabling encoding
auto-encoding. I agree we could ignore it to take the encoding auto-detection
effect when it is enabled. But once user disables the auto-detection, shouldn't
we still respect the encoding user chooses before? 

2) This behavior should be part of of render engine encoding behaviors. We might
need to implement this behavior inside WebKit, then all browser based on WebKit
will have same behavior when disabling/enabling encoding auto-detection.

I suggest the change could be done in TextResourceDecoder.cpp
bool TextResourceDecoder::shouldAutoDetect() const
{
    ...
    return m_usesEncodingDetector
        && (m_source == DefaultEncoding || (m_source == EncodingFromParentFrame
&& m_hintEncoding) ||
            m_source == UserChosenEncoding); 
}

With this change, encoding auto-detection works well when it is enabled, even
user overrides another encoding before. When disabling the encoding
auto-detection, the user chosen encoding is still respected.

Does it make sense?

Thanks!



On 2009/09/10 05:38:56, Roland wrote:
> Hi Johnny,
> 
> implementing it the way you described was my first attempt at implementing
this,
> but it doesn't seem to clear the override encoding if it's set (at least not
for
> me... @_@?)
> 
> E.g., if I choose 'Baltic' in step 4 in my test list, step 5 will then fail:
> it's still 'Baltic' after switching on 'auto-detect', and it still displays
> garbage. 
> 
> 
> On 2009/09/10 01:52:05, Johnny(Jianning) Ding wrote:
> > Hi Roland, Jungshik
> > 
> > Sorry for being late response. Thanks Roland to fix this problem.
> > I like this patch, but I think we could make the change more simple.
> > 
> > Since the flag of using auto detection is controlled on Chrome side and the
> > value will sync to render engine each time it gets changed (see the message:
> > ViewMsg_UpdateWebPreferences). We do not need to send the reset-encoding
> message
> > to render engine. I think a very simple change on browser.cc like the
> following
> > code is enough.
> > void Browser::ToggleEncodingAutoDetect() {
> >   ...
> >   if (encoding_auto_detect_.GetValue())
> >     Reload();
> >   ...
> > }
> > 
> > I have tested it, it works well without changing current logic.
> > What are your opinions?
> > 
> > Thanks again!
> > 
> > 
> > On 2009/09/09 18:45:20, Jungshik Shin wrote:
> > > Brett and Johny, do you have any comment or concern? If not, I'll land
this
> > CL. 
> > > 
> > > On 2009/09/09 01:55:27, Roland wrote:
> > > > On 2009/09/08 17:29:12, Jungshik Shin wrote:
> > > > > With the comments below addressed, it looks good. 
> > > > 
> > > > Uploaded a new patch set with the comments added/changed (no functional
> > > change).
> > > > 
> > > > > If others are ok, I'll land the patch (or can you land yourself,
> Roland?).
> > > > 
> > > > Not yet, working on it... ^_^;

[Roland, 2009-09-10 12:26:19]

On 2009/09/10 08:13:03, Johnny(Jianning) Ding wrote:
> bool TextResourceDecoder::shouldAutoDetect() const
> {
>     ...
>     return m_usesEncodingDetector
>         && (m_source == DefaultEncoding || (m_source ==
EncodingFromParentFrame
> && m_hintEncoding) ||
>             m_source == UserChosenEncoding); 
> }
> 
> With this change, encoding auto-detection works well when it is enabled, even
> user overrides another encoding before. When disabling the encoding
> auto-detection, the user chosen encoding is still respected.

Whoa, I didn't want to open that political can of worms! ^_- 
If we go this direction, then IMHO we should first raise the question on
webkit-dev.

However, before that I think there are a few issues with that approach.
As far a I understand (and please correct me where I may be wrong!):

.) In this way, as long as as auto-detect is turned on, the user cannot override
the encoding anymore. In order to do this, the user (or the system) would need
to turn auto detection off.

.) However, "Auto-detect encoding" is a browser-wide setting, not bound to a
single tab (belongs in the crank menu rather than the page menu, technically,
now that I think of it). So turning it off locally is not possible at the
moment, and turning it off globally would be annoying to the user for the
following reasons:

a.) Changing it to a per-tab basis would open another can of worms in that the
user would have to constantly check and set/re-set it for new tabs (depending on
what we set as default for new tabs and what the user wants).

b.) Turning it off globally would need to answer the question (apart from the
fact that I believe that this would be annoying): what happens if the user turns
it back on for another tab? Either we reload all tabs (not good - the user can't
set encoding on a per-tab basis anymore), or allow all non-active tabs to linger
in an in-between state (auto-detect turned on but not performed) until encoding
detection is triggered again for whatever reason.

.) Generally, just as is the case currently, with this the user cannot get rid
of an override encoding that he has set at some point (this may be a minor issue
though, provided we can address the above points).


This issue with the global auto-detect was one of the main reasons why I chose
the approach of explicitly removing the override encoding.

(Final very minor nit: "calling Reload()" when turning on auto-detect would
currently register a "Reload" user action, which should also be avoided IMHO).

[jungshik at Google, 2009-09-10 18:42:25]

On 2009/09/10 12:26:19, Roland wrote:
> On 2009/09/10 08:13:03, Johnny(Jianning) Ding wrote:
> > bool TextResourceDecoder::shouldAutoDetect() const
> > {
> >     ...
> >     return m_usesEncodingDetector
> >         && (m_source == DefaultEncoding || (m_source ==
> EncodingFromParentFrame
> > && m_hintEncoding) ||
> >             m_source == UserChosenEncoding); 
> > }
> > 
> > With this change, encoding auto-detection works well when it is enabled,
even
> > user overrides another encoding before. 


> When disabling the encoding
> > auto-detection, the user chosen encoding is still respected.

Before actually looking at this CL, I thought Roland had done what you mentioned
in your first comment. It turned out that he'd gone further than that. 

Then, I began to like Roland's implementation because the current Webkit
override-encoding handling is messy and there's no way to clear the override
encoding (see http://bugs.webkit.org/show_bug.cgi?id=17873).  Just applying the
patch (or a version updated to the webkit trunk with a webkit preference as
talked about toward the end of that bug) in webkit bug 17873 has its own problem
(I considered making a local change with that approach for Chrome 3.0, but gave
it up). It seems that an ultimate solution to webkit bug 17873 is to change the
way the override encoding is stored in webkit.

In the meantime, I don't think it's necessary to go back to the override
encoding when auto-detection is turned off. Actually, in many cases, it may be
better to go back to the default encoding (by clearing the override encoding). 




> Whoa, I didn't want to open that political can of worms! ^_- 
> If we go this direction, then IMHO we should first raise the question on
> webkit-dev.
> 
> However, before that I think there are a few issues with that approach.
> As far a I understand (and please correct me where I may be wrong!):
> 
> .) In this way, as long as as auto-detect is turned on, the user cannot
override
> the encoding anymore. In order to do this, the user (or the system) would need
> to turn auto detection off.

Actually, that's not the case. Even with auto-detect on, a user can still
manually override the encoding. A user always has the final say.
 
> .) However, "Auto-detect encoding" is a browser-wide setting, not bound to a
> single tab (belongs in the crank menu rather than the page menu, technically,
> now that I think of it). So turning it off locally is not possible at the
> moment, and turning it off globally would be annoying to the user for the
> following reasons:
> 
> a.) Changing it to a per-tab basis would open another can of worms in that the
> user would have to constantly check and set/re-set it for new tabs (depending
on
> what we set as default for new tabs and what the user wants).
> 
> b.) Turning it off globally would need to answer the question (apart from the
> fact that I believe that this would be annoying): what happens if the user
turns
> it back on for another tab? Either we reload all tabs (not good - the user
can't
> set encoding on a per-tab basis anymore), or allow all non-active tabs to
linger
> in an in-between state (auto-detect turned on but not performed) until
encoding
> detection is triggered again for whatever reason.
> 
> .) Generally, just as is the case currently, with this the user cannot get rid
> of an override encoding that he has set at some point (this may be a minor
issue
> though, provided we can address the above points).
> 
> 
> This issue with the global auto-detect was one of the main reasons why I chose
> the approach of explicitly removing the override encoding.
> 
> (Final very minor nit: "calling Reload()" when turning on auto-detect would
> currently register a "Reload" user action, which should also be avoided IMHO).

[Johnny(Jianning) Ding, 2009-09-11 04:50:39]

Roland, Jungshik
Thanks for explaining the underlying reasons of this CL.
The CL is LGTM now:-)

One suggestion, since the encoding UI test is alive, would you please write a
test case in browser_encoding_uitest.cc? Your description in TEST section can be
easily written to a test case.


On 2009/09/10 18:42:25, Jungshik Shin wrote:
> On 2009/09/10 12:26:19, Roland wrote:
> > On 2009/09/10 08:13:03, Johnny(Jianning) Ding wrote:
> > > bool TextResourceDecoder::shouldAutoDetect() const
> > > {
> > >     ...
> > >     return m_usesEncodingDetector
> > >         && (m_source == DefaultEncoding || (m_source ==
> > EncodingFromParentFrame
> > > && m_hintEncoding) ||
> > >             m_source == UserChosenEncoding); 
> > > }
> > > 
> > > With this change, encoding auto-detection works well when it is enabled,
> even
> > > user overrides another encoding before. 
> 
> 
> > When disabling the encoding
> > > auto-detection, the user chosen encoding is still respected.
> 
> Before actually looking at this CL, I thought Roland had done what you
mentioned
> in your first comment. It turned out that he'd gone further than that. 
> 
> Then, I began to like Roland's implementation because the current Webkit
> override-encoding handling is messy and there's no way to clear the override
> encoding (see http://bugs.webkit.org/show_bug.cgi?id=17873).  Just applying
the
> patch (or a version updated to the webkit trunk with a webkit preference as
> talked about toward the end of that bug) in webkit bug 17873 has its own
problem
> (I considered making a local change with that approach for Chrome 3.0, but
gave
> it up). It seems that an ultimate solution to webkit bug 17873 is to change
the
> way the override encoding is stored in webkit.
> 
> In the meantime, I don't think it's necessary to go back to the override
> encoding when auto-detection is turned off. Actually, in many cases, it may be
> better to go back to the default encoding (by clearing the override encoding).

> 
> 
> 
> 
> > Whoa, I didn't want to open that political can of worms! ^_- 
> > If we go this direction, then IMHO we should first raise the question on
> > webkit-dev.
> > 
> > However, before that I think there are a few issues with that approach.
> > As far a I understand (and please correct me where I may be wrong!):
> > 
> > .) In this way, as long as as auto-detect is turned on, the user cannot
> override
> > the encoding anymore. In order to do this, the user (or the system) would
need
> > to turn auto detection off.
> 
> Actually, that's not the case. Even with auto-detect on, a user can still
> manually override the encoding. A user always has the final say.
>  
> > .) However, "Auto-detect encoding" is a browser-wide setting, not bound to a
> > single tab (belongs in the crank menu rather than the page menu,
technically,
> > now that I think of it). So turning it off locally is not possible at the
> > moment, and turning it off globally would be annoying to the user for the
> > following reasons:
> > 
> > a.) Changing it to a per-tab basis would open another can of worms in that
the
> > user would have to constantly check and set/re-set it for new tabs
(depending
> on
> > what we set as default for new tabs and what the user wants).
> > 
> > b.) Turning it off globally would need to answer the question (apart from
the
> > fact that I believe that this would be annoying): what happens if the user
> turns
> > it back on for another tab? Either we reload all tabs (not good - the user
> can't
> > set encoding on a per-tab basis anymore), or allow all non-active tabs to
> linger
> > in an in-between state (auto-detect turned on but not performed) until
> encoding
> > detection is triggered again for whatever reason.
> > 
> > .) Generally, just as is the case currently, with this the user cannot get
rid
> > of an override encoding that he has set at some point (this may be a minor
> issue
> > though, provided we can address the above points).
> > 
> > 
> > This issue with the global auto-detect was one of the main reasons why I
chose
> > the approach of explicitly removing the override encoding.
> > 
> > (Final very minor nit: "calling Reload()" when turning on auto-detect would
> > currently register a "Reload" user action, which should also be avoided
IMHO).

[Roland, 2009-09-11 06:44:00]

On 2009/09/11 04:50:39, Johnny(Jianning) Ding wrote:
> Roland, Jungshik
> Thanks for explaining the underlying reasons of this CL.
> The CL is LGTM now:-)

Thanks! ^_^

> One suggestion, since the encoding UI test is alive, would you please write a
> test case in browser_encoding_uitest.cc? Your description in TEST section can
be
> easily written to a test case.

Hm, I'm not yet too familiar with the automation code, but looking at
browser_encoding_uitest.cc, in order to toggle the auto-detect, the existing
code does

browser>SetBooleanPreference(
  prefs::kWebKitUsesUniversalDetector,
  false)
);
EXPECT_TRUE(tab->Reload());

which is obviously counterproductive when testing that a reload does not in fact
occur. ^_-

Therefore I would propose to extend the automation interface with
ToggleEncodingAutoDetect(), in the lines of OverrideEncoding() (see
tab_proxy.h).

I have already started implementing this, but this would touch yet another bunch
of files.

So if it's alright with you guys I would rather keep the patch size reasonable
and put that new automation interface (and the new test) under a new bug entry.

[jungshik at Google, 2009-09-11 17:09:28]

Thank you, Roland and Johny. Assuming that Roland will add a ui test soon (he
already started), I'll land this CL on his behalf.

[Johnny(Jianning) Ding, 2009-09-11 18:41:18]

2009/9/11 <rolandsteiner@google.com>

> On 2009/09/11 04:50:39, Johnny(Jianning) Ding wrote:
>
>> Roland, Jungshik
>> Thanks for explaining the underlying reasons of this CL.
>> The CL is LGTM now:-)
>>
>
> Thanks! ^_^
>
>  One suggestion, since the encoding UI test is alive, would you please
>> write a
>> test case in browser_encoding_uitest.cc? Your description in TEST section
>> can
>>
> be
>
>> easily written to a test case.
>>
>
> Hm, I'm not yet too familiar with the automation code, but looking at
> browser_encoding_uitest.cc, in order to toggle the auto-detect, the
> existing
> code does
>
> browser>SetBooleanPreference(
>  prefs::kWebKitUsesUniversalDetector,
>  false)
> );
> EXPECT_TRUE(tab->Reload());
>
In this test logic,
1) When first time disabling the auto-detect , the reload does not do wrong
thing, we just want to make sure current encoding is default encoding
2) After then when enabling auto-detect, since we will use a encoding reset
message instead of calling reload, yes, you need to have a new automation
interface say ResetEncodingToDefault, or maybe you can write a interface
called ToggleEncodingAutoDetect to either
set prefs::kWebKitUsesUniversalDetector + reset encoding or only
clear prefs::kWebKitUsesUniversalDetector. Then the test code will make sure
the page encoding is right contents encoding.
3) After then you need to add new logic to disable the auto-detect, then the
test code will make sure the page encoding does not change (That is why you
write this CL to fix bug 2932)


> which is obviously counterproductive when testing that a reload does not in
> fact
> occur. ^_-
>
> Therefore I would propose to extend the automation interface with
> ToggleEncodingAutoDetect(), in the lines of OverrideEncoding() (see
> tab_proxy.h).
>
> I have already started implementing this, but this would touch yet another
> bunch
> of files.
>
> So if it's alright with you guys I would rather keep the patch size
> reasonable
> and put that new automation interface (and the new test) under a new bug
> entry.

I am OK with writing the test in new patch. So far your CL should not break
the BrowserEncodingTest::TestEncodingAutoDetect because it does not check
whether the encoding will be changed when changing auto-detect from enable
status to disable.

>
>
>
>
> http://codereview.chromium.org/173265
>



-- 
Best Regards.
Johnny

[jungshik at Google, 2009-09-15 17:21:37]

Landed : 
http://src.chromium.org/viewvc/chrome?view=rev&revision=26168

Thanks !