Translate: infobars should not appear when a page has a refresh meta tag

Translate: infobars should not appear when a page has a refresh meta tag

If a page contains a <meta http-equiv="refresh" content="0;...">,
a client redirect is performed, but |page_id| is not modified.

On the other hand, CapturePageInfo is executed as a delayed function.
This function may handle a different page if user took an action to
navigate a page, or a page is navigated automatically by a script or
a client redirect. The page can be incomplete when this function runs.
There, |page_id| can be a hint to distinguish pages and abort functions.
But if the page is redirected by refresh meta tag, it is useless.

This change omit to call CapturePageInfo if a loaded page contains
refresh meta tag to avoid TranslateHelper is confused.

BUG=236781
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201645

[Takashi Toyoshima, 2013-05-01 12:17:18]

(CCed to mad and miguelg as developers on Chrome Translate)

Hi sky,

Can you take an OWNERS review on ChromeRenderViewObserver?
I ran git annotate on the file, but It looks a little difficult to find a proper
OWNERS reviewer. If you are not familiar with this class, can you reassign
someone who may know better this change?

Thanks in advance.

[sky, 2013-05-01 16:14:04]

I'm not a good OWNER here. Maybe John is?

[jam, 2013-05-01 19:35:22]

On 2013/05/01 16:14:04, sky wrote:
> I'm not a good OWNER here. Maybe John is?

I don't have the bandwidth to do reviews in src/chrome, please find an owner in
src/chrome/OWNERS

[Takashi Toyoshima, 2013-05-02 01:38:38]

+jochen, brettw

I'm finding a proper OWNERS reviewer for
chrome/renderer/chrome_render_view_observer.
Can one of you guys take a look this?
It's a small patch of ~40lines.

Thanks in advance.

[jochen (gone - plz use gerrit), 2013-05-02 09:48:42]

https://codereview.chromium.org/14769004/diff/1/chrome/renderer/chrome_render...
File chrome/renderer/chrome_render_view_observer.cc (left):

https://codereview.chromium.org/14769004/diff/1/chrome/renderer/chrome_render...
chrome/renderer/chrome_render_view_observer.cc:674: int page_id =
render_view()->GetPageId();
what's the reason to not just check for the meta tag here?

[Takashi Toyoshima, 2013-05-02 11:51:18]

https://codereview.chromium.org/14769004/diff/1/chrome/renderer/chrome_render...
File chrome/renderer/chrome_render_view_observer.cc (left):

https://codereview.chromium.org/14769004/diff/1/chrome/renderer/chrome_render...
chrome/renderer/chrome_render_view_observer.cc:674: int page_id =
render_view()->GetPageId();
Thanks.
You are right. I should just pass page_id and check the id is not changed here.
Refresh meta information and captured text should be consistent with page_id.

But, after revised my change, I find another problem. I'm not sure the reason
now, but I can not find refresh meta tag here in my test case using <meta
http-equiv="refresh" content="0;url=...>.

PageCapture returns apparently the first page contents, but refresh meta tag
disappears. I guess another code remove the tag when it handles the client
redirect.

I'll continue to investigate this problem.

[Takashi Toyoshima, 2013-05-02 12:13:57]

https://codereview.chromium.org/14769004/diff/1/chrome/renderer/chrome_render...
File chrome/renderer/chrome_render_view_observer.cc (left):

https://codereview.chromium.org/14769004/diff/1/chrome/renderer/chrome_render...
chrome/renderer/chrome_render_view_observer.cc:674: int page_id =
render_view()->GetPageId();
In the case of client redirect of meta refresh, page_id is not updated.

If a redirected second page is slow, we may capture incomplete document here. In
my case, I captured incomplete empty body and can not find refresh meta because
in this function, client redirect is already performed, and the second page is
waiting for loaded.

So, we should check refresh meta tag before posting task as my first change did.

[Takashi Toyoshima, 2013-05-02 12:50:07]

jochen,

I upload Patch Set 3. This is based on the first patch, but I refined it because
now I correctly understand what happens there. Also I updated change
description.

Can you take a look again? We're targeting m28.

[jochen (gone - plz use gerrit), 2013-05-02 14:00:31]

can you write a test for this?

What happens if the meta tag is inserted by javascript right after the document
finished loading?

Note that finish loading means "all bytes arrived", the document might still be
parsing, and so an onload handler could still create such a tag if I'm not
mistaken.

What if the javascript just changes the location before (again after load
finished, before the onload event)?

https://codereview.chromium.org/14769004/diff/21001/chrome/renderer/chrome_re...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://codereview.chromium.org/14769004/diff/21001/chrome/renderer/chrome_re...
chrome/renderer/chrome_render_view_observer.cc:198: bool
ChromeRenderViewObserver::HasRefreshMetaTag(WebFrame* frame) {
why is this static?

[Takashi Toyoshima, 2013-05-02 14:48:13]

> can you write a test for this?

Ok, I'll try... but time looks running out. (Holiday starts after minutes in
Japan)
Anyway, I'll try to write it tomorrow morning.
Do you think it's fine to have it as a browser test in
chrome/browser/translate/translate_browsertest.cc ?

> What happens if the meta tag is inserted by javascript right after the
document
> finished loading?

Still we can not handle this case correctly. But this is very rare case and
uncommon usage. I don't think we should save all cases here. FYI, this case,
Translate does nothing against redirected page.

> What if the javascript just changes the location before (again after load
> finished, before the onload event)?

Modifying location updates page_id. So we can notice the page is navigated.
In my patch set 4, just returns without doing anything.
Also Translate aborts current operations here and there on noticing that the
page_id is changed.

>
https://codereview.chromium.org/14769004/diff/21001/chrome/renderer/chrome_re...
> chrome/renderer/chrome_render_view_observer.cc:198: bool
> ChromeRenderViewObserver::HasRefreshMetaTag(WebFrame* frame) {
> why is this static?

Just because this function doesn't need |this|. I can change it to a private
method.

[jochen (gone - plz use gerrit), 2013-05-02 15:10:47]

On 2013/05/02 14:48:13, Takashi Toyoshima (chromium) wrote:
> > can you write a test for this?
> 
> Ok, I'll try... but time looks running out. (Holiday starts after minutes in
> Japan)

I don't think that the branch time is a good argument for hacking around bugs.
Either it's an important fix, then we can merge it after the branch, or it can
wait.

> Anyway, I'll try to write it tomorrow morning.
> Do you think it's fine to have it as a browser test in
> chrome/browser/translate/translate_browsertest.cc ?

Sure.

> 
> > What happens if the meta tag is inserted by javascript right after the
> document
> > finished loading?
> 
> Still we can not handle this case correctly. But this is very rare case and
> uncommon usage. I don't think we should save all cases here. FYI, this case,
> Translate does nothing against redirected page.
> 
> > What if the javascript just changes the location before (again after load
> > finished, before the onload event)?
> 
> Modifying location updates page_id. So we can notice the page is navigated.
> In my patch set 4, just returns without doing anything.
> Also Translate aborts current operations here and there on noticing that the
> page_id is changed.

It shouldn't change the page id if you modify the location during the onload
handler. But I might be mistaken..

> 
> >
>
https://codereview.chromium.org/14769004/diff/21001/chrome/renderer/chrome_re...
> > chrome/renderer/chrome_render_view_observer.cc:198: bool
> > ChromeRenderViewObserver::HasRefreshMetaTag(WebFrame* frame) {
> > why is this static?
> 
> Just because this function doesn't need |this|. I can change it to a private
> method.

it needs the WebFrame, if you use this, you can just access it via render_view()

[Takashi Toyoshima, 2013-05-02 16:18:52]

Sorry, I didn't mean that I don't want to write a test, or must land it by m28.
This change seems to have many side effects. So it's safe and better to land
after the next branch.

Also, I should understand more details about page_id. I supposed this value is
equivalent to the index of navigation history. Is this understanding right?

Understanding page_id and page navigation is important to me because I'm not
sure how Translate infobar should behave when location or pushState is modified.

Thank you for many useful comments.

[jochen (gone - plz use gerrit), 2013-05-03 11:24:48]

On 2013/05/02 16:18:52, Takashi Toyoshima (chromium) wrote:
> Sorry, I didn't mean that I don't want to write a test, or must land it by
m28.
> This change seems to have many side effects. So it's safe and better to land
> after the next branch.
> 
> Also, I should understand more details about page_id. I supposed this value is
> equivalent to the index of navigation history. Is this understanding right?
> 
> Understanding page_id and page navigation is important to me because I'm not
> sure how Translate infobar should behave when location or pushState is
modified.


The current location in the session history is stored in
RenderViewImpl::history_list_offset_ (however, that value is only correct if
that RV is "current", otherwise, only the browser has the correct offset).

e.g. if a page just changes it's reference fragment, the page id doesn't change
but the history list offset does.

I guess the correct behavior depends on when you want to display the infobar,
and what content it should capture.

The session history is updated, when the RV commits (this also happens if e.g.
history.pushState() is invoked).

At this point, you probably want to either schedule capturing the page info (if
the page id didn't change), or (if it changed), wait for the document to finish
parsing (RenderViewImpl::didFinishDocumentLoad)

> 
> Thank you for many useful comments.

[Takashi Toyoshima, 2013-05-14 14:38:29]

Hi, I added some browser tests.

1. a page having http-equiv=refresh meta tag
2. a page that http-equiv=refresh meta tag is injected at onload
3. a page that location is updated by inline JavaScript
4. a page that location is updated by JavaScript at onload

All cases work. 1, 3, and 4 is expected result. But I don't have confidence 2.
always work fine. It might be flaky test. If it will be flaky, I'll remove it
because I didn't suppose that my change fix that case.

I also check updating location by setTimeout with 1000 msec works. But I didn't
add it to browser test because it is slow.

Thanks.

https://chromiumcodereview.appspot.com/14769004/diff/30001/chrome/renderer/ch...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://chromiumcodereview.appspot.com/14769004/diff/30001/chrome/renderer/ch...
chrome/renderer/chrome_render_view_observer.cc:204: return false;
webkit_glue::GetMetaElementsWithAttribute is removed.
I implement it here.

https://chromiumcodereview.appspot.com/14769004/diff/35001/chrome/renderer/ch...
File chrome/renderer/chrome_render_view_observer.h (right):

https://chromiumcodereview.appspot.com/14769004/diff/35001/chrome/renderer/ch...
chrome/renderer/chrome_render_view_observer.h:166: bool
HasRefreshMetaTag(WebKit::WebFrame* frame);
We still need WebFrame argument because two kinds of WebFrame can be passed. One
is real WebFrame which can be accessed via render_view(), and the other is
provisional one which is passed at DidCommitProvisionalLoad.

[Takashi Toyoshima, 2013-05-16 04:08:08]

jochen: ping

[jochen (gone - plz use gerrit), 2013-05-22 12:26:02]

lgtm

https://chromiumcodereview.appspot.com/14769004/diff/42001/chrome/renderer/ch...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://chromiumcodereview.appspot.com/14769004/diff/42001/chrome/renderer/ch...
chrome/renderer/chrome_render_view_observer.cc:814: const extensions::Extension*
ChromeRenderViewObserver::GetExtension(
any reason you move this method?

[Takashi Toyoshima, 2013-05-22 12:59:39]

Thanks.

https://chromiumcodereview.appspot.com/14769004/diff/42001/chrome/renderer/ch...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://chromiumcodereview.appspot.com/14769004/diff/42001/chrome/renderer/ch...
chrome/renderer/chrome_render_view_observer.cc:814: const extensions::Extension*
ChromeRenderViewObserver::GetExtension(
This is because only this function is inconsistent with the function order in
the header file, and here looks proper place to be.

[commit-bot: I haz the power, 2013-05-22 13:05:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/14769004/61003

[commit-bot: I haz the power, 2013-05-23 00:10:10]

Change committed as 201645