Add a unit test for refresh in mshtml to see where we're not tagging the user...

Add a unit test for refresh in mshtml to see where we're not tagging the user agent

TEST=The test currently fails, but you can run it!
BUG=55758
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=60838

[robertshield, 2010-09-27 17:26:23]

Some nits. Feel free to ignore the token-replacement design nits for this CL,
but I feel they should be addressed soon-ish before they get out of hand.

http://codereview.chromium.org/3433026/diff/1/3
File chrome_frame/test/data/mshtml_refresh_test_popup.html (right):

http://codereview.chromium.org/3433026/diff/1/3#newcode6
chrome_frame/test/data/mshtml_refresh_test_popup.html:6: //
window.opener.location.href = window.opener.location.href;
Do we want to add a test that this is indeed the same behaviour?

http://codereview.chromium.org/3433026/diff/1/4
File chrome_frame/test/test_with_web_server.cc (right):

http://codereview.chromium.org/3433026/diff/1/4#newcode1035
chrome_frame/test/test_with_web_server.cc:1035: class UaTemplateFileResponse :
public test_server::FileResponse {
While this is fine for this CL, I'm a little worried about the design here. It
leads to there being no single place where the replacement tokens for the test
html pages are defined which isn't very maintainable.

It would be cleaner to have a single token replacement mechanism.

http://codereview.chromium.org/3433026/diff/1/4#newcode1048
chrome_frame/test/test_with_web_server.cc:1048: virtual size_t ContentLength()
const {
Also a ContentLength() accessor isn't the first place I'd look for token
replacement code ;-) The const on this method is a lie.

http://codereview.chromium.org/3433026/diff/1/4#newcode1052
chrome_frame/test/test_with_web_server.cc:1052: size_t length =
__super::ContentLength();
I know this is unlikely to be compiled on non-msvc, but could we have a 
typedef test_server::FileResponse Super;
near the top instead?

http://codereview.chromium.org/3433026/diff/1/4#newcode1092
chrome_frame/test/test_with_web_server.cc:1092: SimpleWebServerTest
server(46664);
could this devilish looking number be constantized? it is used in a few places
here...

http://codereview.chromium.org/3433026/diff/1/4#newcode1094
chrome_frame/test/test_with_web_server.cc:1094: arraysize(kPages),
GetCFTestFilePath());
I think you should either have wrappers for all of the specializations of
PopulateStaticFileListT or for none of them.

[amit, 2010-09-27 17:35:28]

On Mon, Sep 27, 2010 at 10:26 AM, <robertshield@chromium.org> wrote:

> Some nits. Feel free to ignore the token-replacement design nits for this
> CL,
> but I feel they should be addressed soon-ish before they get out of hand.
>
>
> http://codereview.chromium.org/3433026/diff/1/3
> File chrome_frame/test/data/mshtml_refresh_test_popup.html (right):
>
> http://codereview.chromium.org/3433026/diff/1/3#newcode6
> chrome_frame/test/data/mshtml_refresh_test_popup.html:6: //
> window.opener.location.href = window.opener.location.href;
> Do we want to add a test that this is indeed the same behaviour?
>
> http://codereview.chromium.org/3433026/diff/1/4
> File chrome_frame/test/test_with_web_server.cc (right):
>
> http://codereview.chromium.org/3433026/diff/1/4#newcode1035
> chrome_frame/test/test_with_web_server.cc:1035: class
> UaTemplateFileResponse : public test_server::FileResponse {
> While this is fine for this CL, I'm a little worried about the design
> here. It leads to there being no single place where the replacement
> tokens for the test html pages are defined which isn't very
> maintainable.
>
> It would be cleaner to have a single token replacement mechanism.
>
Is it possible to use mock web server here?


>
> http://codereview.chromium.org/3433026/diff/1/4#newcode1048
> chrome_frame/test/test_with_web_server.cc:1048: virtual size_t
> ContentLength() const {
> Also a ContentLength() accessor isn't the first place I'd look for token
> replacement code ;-) The const on this method is a lie.
>
> http://codereview.chromium.org/3433026/diff/1/4#newcode1052
> chrome_frame/test/test_with_web_server.cc:1052: size_t length =
> __super::ContentLength();
> I know this is unlikely to be compiled on non-msvc, but could we have a
> typedef test_server::FileResponse Super;
> near the top instead?
>
> http://codereview.chromium.org/3433026/diff/1/4#newcode1092
> chrome_frame/test/test_with_web_server.cc:1092: SimpleWebServerTest
> server(46664);
> could this devilish looking number be constantized? it is used in a few
> places here...
>
> http://codereview.chromium.org/3433026/diff/1/4#newcode1094
> chrome_frame/test/test_with_web_server.cc:1094: arraysize(kPages),
> GetCFTestFilePath());
> I think you should either have wrappers for all of the specializations
> of PopulateStaticFileListT or for none of them.
>
>
> http://codereview.chromium.org/3433026/show
>

[tommi (sloooow) - chröme, 2010-09-27 17:45:47]

http://codereview.chromium.org/3433026/diff/1/3
File chrome_frame/test/data/mshtml_refresh_test_popup.html (right):

http://codereview.chromium.org/3433026/diff/1/3#newcode6
chrome_frame/test/data/mshtml_refresh_test_popup.html:6: //
window.opener.location.href = window.opener.location.href;
On 2010/09/27 17:26:23, robertshield wrote:
> Do we want to add a test that this is indeed the same behaviour?
There's no need.  The fact that reloads behave this way is enough.

http://codereview.chromium.org/3433026/diff/1/4
File chrome_frame/test/test_with_web_server.cc (right):

http://codereview.chromium.org/3433026/diff/1/4#newcode1035
chrome_frame/test/test_with_web_server.cc:1035: class UaTemplateFileResponse :
public test_server::FileResponse {
On 2010/09/27 17:26:23, robertshield wrote:
> While this is fine for this CL, I'm a little worried about the design here. It
> leads to there being no single place where the replacement tokens for the test
> html pages are defined which isn't very maintainable.
> 
> It would be cleaner to have a single token replacement mechanism.

Yes. Do we have one?  I wouldn't really call this a design, but I can imagine
that we might find something like this useful in the future.

http://codereview.chromium.org/3433026/diff/1/4#newcode1048
chrome_frame/test/test_with_web_server.cc:1048: virtual size_t ContentLength()
const {
On 2010/09/27 17:26:23, robertshield wrote:
> Also a ContentLength() accessor isn't the first place I'd look for token
> replacement code ;-) The const on this method is a lie.

The const on many of these methods are a lie.  The interface shouldn't really be
specified as const since it assumes how it will be implemented.

http://codereview.chromium.org/3433026/diff/1/4#newcode1052
chrome_frame/test/test_with_web_server.cc:1052: size_t length =
__super::ContentLength();
On 2010/09/27 17:26:23, robertshield wrote:
> I know this is unlikely to be compiled on non-msvc, but could we have a 
> typedef test_server::FileResponse Super;
> near the top instead?

Done.

http://codereview.chromium.org/3433026/diff/1/4#newcode1092
chrome_frame/test/test_with_web_server.cc:1092: SimpleWebServerTest
server(46664);
On 2010/09/27 17:26:23, robertshield wrote:
> could this devilish looking number be constantized? it is used in a few places
> here...
devilish?  It's gooog :)

http://codereview.chromium.org/3433026/diff/1/4#newcode1094
chrome_frame/test/test_with_web_server.cc:1094: arraysize(kPages),
GetCFTestFilePath());
On 2010/09/27 17:26:23, robertshield wrote:
> I think you should either have wrappers for all of the specializations of
> PopulateStaticFileListT or for none of them.

Feeling a bit binary today? :)  I don't agree.
If you can point to an example where there's a template class or function and
all of the possible template specializations are defined, then I might consider
it ;)

[tommi (sloooow) - chröme, 2010-09-27 18:31:02]

I think so, but it will take some time and I'd have to do the templating etc
anyway.
Do you think it is worth it?

On Mon, Sep 27, 2010 at 1:35 PM, Amit Joshi <amit@chromium.org> wrote:

>
>
> On Mon, Sep 27, 2010 at 10:26 AM, <robertshield@chromium.org> wrote:
>
>> Some nits. Feel free to ignore the token-replacement design nits for this
>> CL,
>> but I feel they should be addressed soon-ish before they get out of hand.
>>
>>
>> http://codereview.chromium.org/3433026/diff/1/3
>> File chrome_frame/test/data/mshtml_refresh_test_popup.html (right):
>>
>> http://codereview.chromium.org/3433026/diff/1/3#newcode6
>> chrome_frame/test/data/mshtml_refresh_test_popup.html:6: //
>> window.opener.location.href = window.opener.location.href;
>> Do we want to add a test that this is indeed the same behaviour?
>>
>> http://codereview.chromium.org/3433026/diff/1/4
>> File chrome_frame/test/test_with_web_server.cc (right):
>>
>> http://codereview.chromium.org/3433026/diff/1/4#newcode1035
>> chrome_frame/test/test_with_web_server.cc:1035: class
>> UaTemplateFileResponse : public test_server::FileResponse {
>> While this is fine for this CL, I'm a little worried about the design
>> here. It leads to there being no single place where the replacement
>> tokens for the test html pages are defined which isn't very
>> maintainable.
>>
>> It would be cleaner to have a single token replacement mechanism.
>>
> Is it possible to use mock web server here?
>
>
>>
>> http://codereview.chromium.org/3433026/diff/1/4#newcode1048
>> chrome_frame/test/test_with_web_server.cc:1048: virtual size_t
>> ContentLength() const {
>> Also a ContentLength() accessor isn't the first place I'd look for token
>> replacement code ;-) The const on this method is a lie.
>>
>> http://codereview.chromium.org/3433026/diff/1/4#newcode1052
>> chrome_frame/test/test_with_web_server.cc:1052: size_t length =
>> __super::ContentLength();
>> I know this is unlikely to be compiled on non-msvc, but could we have a
>> typedef test_server::FileResponse Super;
>> near the top instead?
>>
>> http://codereview.chromium.org/3433026/diff/1/4#newcode1092
>> chrome_frame/test/test_with_web_server.cc:1092: SimpleWebServerTest
>> server(46664);
>> could this devilish looking number be constantized? it is used in a few
>> places here...
>>
>> http://codereview.chromium.org/3433026/diff/1/4#newcode1094
>> chrome_frame/test/test_with_web_server.cc:1094: arraysize(kPages),
>> GetCFTestFilePath());
>> I think you should either have wrappers for all of the specializations
>> of PopulateStaticFileListT or for none of them.
>>
>>
>> http://codereview.chromium.org/3433026/show
>>
>
>

[robertshield, 2010-09-27 18:50:14]

LGTM.

http://codereview.chromium.org/3433026/diff/1/3
File chrome_frame/test/data/mshtml_refresh_test_popup.html (right):

http://codereview.chromium.org/3433026/diff/1/3#newcode6
chrome_frame/test/data/mshtml_refresh_test_popup.html:6: //
window.opener.location.href = window.opener.location.href;
On 2010/09/27 17:45:48, tommi wrote:
> On 2010/09/27 17:26:23, robertshield wrote:
> > Do we want to add a test that this is indeed the same behaviour?
> There's no need.  The fact that reloads behave this way is enough.
> 

ok, cool

http://codereview.chromium.org/3433026/diff/1/4
File chrome_frame/test/test_with_web_server.cc (right):

http://codereview.chromium.org/3433026/diff/1/4#newcode1094
chrome_frame/test/test_with_web_server.cc:1094: arraysize(kPages),
GetCFTestFilePath());
On 2010/09/27 17:45:48, tommi wrote:
> On 2010/09/27 17:26:23, robertshield wrote:
> > I think you should either have wrappers for all of the specializations of
> > PopulateStaticFileListT or for none of them.
> 
> Feeling a bit binary today? :)  I don't agree.
> If you can point to an example where there's a template class or function and
> all of the possible template specializations are defined, then I might
consider
> it ;)

heh, no.. I feel it would be more consistent to not mix wrappers and explicit
specializations, but I'm not going to fight about it, nossir.

[tommi (sloooow) - chröme, 2010-09-27 19:00:36]

On Mon, Sep 27, 2010 at 2:50 PM, <robertshield@chromium.org> wrote:

> LGTM.
>
>
>
> http://codereview.chromium.org/3433026/diff/1/3
> File chrome_frame/test/data/mshtml_refresh_test_popup.html (right):
>
> http://codereview.chromium.org/3433026/diff/1/3#newcode6
> chrome_frame/test/data/mshtml_refresh_test_popup.html:6: //
> window.opener.location.href = window.opener.location.href;
> On 2010/09/27 17:45:48, tommi wrote:
>
>> On 2010/09/27 17:26:23, robertshield wrote:
>> > Do we want to add a test that this is indeed the same behaviour?
>> There's no need.  The fact that reloads behave this way is enough.
>>
>
>
> ok, cool
>
>
> http://codereview.chromium.org/3433026/diff/1/4
> File chrome_frame/test/test_with_web_server.cc (right):
>
> http://codereview.chromium.org/3433026/diff/1/4#newcode1094
> chrome_frame/test/test_with_web_server.cc:1094: arraysize(kPages),
> GetCFTestFilePath());
> On 2010/09/27 17:45:48, tommi wrote:
>
>> On 2010/09/27 17:26:23, robertshield wrote:
>> > I think you should either have wrappers for all of the
>>
> specializations of
>
>> > PopulateStaticFileListT or for none of them.
>>
>
>  Feeling a bit binary today? :)  I don't agree.
>> If you can point to an example where there's a template class or
>>
> function and
>
>> all of the possible template specializations are defined, then I might
>>
> consider
>
>> it ;)
>>
>
> heh, no.. I feel it would be more consistent to not mix wrappers and
> explicit specializations, but I'm not going to fight about it, nossir.


OKOKOK.... enough already!  *taptap*!
done!  feel better?!?!?!  X)

>
>
> http://codereview.chromium.org/3433026/show
>

[amit, 2010-09-27 20:39:37]

On Mon, Sep 27, 2010 at 11:30 AM, Tommi <tommi@chromium.org> wrote:

> I think so, but it will take some time and I'd have to do the templating
> etc anyway.
> Do you think it is worth it?
>
I am assuming this is in response to the mock web server question :)
In that case I feel that using mock server will be a totally different
approach and would allow you the contain the test expectations in the test
itself instead of having a class like UaTemplateFileResponse I was curious
to see if that's doable for this test. If it is doable and not too painful,
then it's better to use/enhance our mock web server.

>
> On Mon, Sep 27, 2010 at 1:35 PM, Amit Joshi <amit@chromium.org> wrote:
>
>>
>>
>> On Mon, Sep 27, 2010 at 10:26 AM, <robertshield@chromium.org> wrote:
>>
>>> Some nits. Feel free to ignore the token-replacement design nits for this
>>> CL,
>>> but I feel they should be addressed soon-ish before they get out of hand.
>>>
>>>
>>> http://codereview.chromium.org/3433026/diff/1/3
>>> File chrome_frame/test/data/mshtml_refresh_test_popup.html (right):
>>>
>>> http://codereview.chromium.org/3433026/diff/1/3#newcode6
>>> chrome_frame/test/data/mshtml_refresh_test_popup.html:6: //
>>> window.opener.location.href = window.opener.location.href;
>>> Do we want to add a test that this is indeed the same behaviour?
>>>
>>> http://codereview.chromium.org/3433026/diff/1/4
>>> File chrome_frame/test/test_with_web_server.cc (right):
>>>
>>> http://codereview.chromium.org/3433026/diff/1/4#newcode1035
>>> chrome_frame/test/test_with_web_server.cc:1035: class
>>> UaTemplateFileResponse : public test_server::FileResponse {
>>> While this is fine for this CL, I'm a little worried about the design
>>> here. It leads to there being no single place where the replacement
>>> tokens for the test html pages are defined which isn't very
>>> maintainable.
>>>
>>> It would be cleaner to have a single token replacement mechanism.
>>>
>> Is it possible to use mock web server here?
>>
>>
>>>
>>> http://codereview.chromium.org/3433026/diff/1/4#newcode1048
>>> chrome_frame/test/test_with_web_server.cc:1048: virtual size_t
>>> ContentLength() const {
>>> Also a ContentLength() accessor isn't the first place I'd look for token
>>> replacement code ;-) The const on this method is a lie.
>>>
>>> http://codereview.chromium.org/3433026/diff/1/4#newcode1052
>>> chrome_frame/test/test_with_web_server.cc:1052: size_t length =
>>> __super::ContentLength();
>>> I know this is unlikely to be compiled on non-msvc, but could we have a
>>> typedef test_server::FileResponse Super;
>>> near the top instead?
>>>
>>> http://codereview.chromium.org/3433026/diff/1/4#newcode1092
>>> chrome_frame/test/test_with_web_server.cc:1092: SimpleWebServerTest
>>> server(46664);
>>> could this devilish looking number be constantized? it is used in a few
>>> places here...
>>>
>>> http://codereview.chromium.org/3433026/diff/1/4#newcode1094
>>> chrome_frame/test/test_with_web_server.cc:1094: arraysize(kPages),
>>> GetCFTestFilePath());
>>> I think you should either have wrappers for all of the specializations
>>> of PopulateStaticFileListT or for none of them.
>>>
>>>
>>> http://codereview.chromium.org/3433026/show
>>>
>>
>>
>