Fix pushState causing stop/reload button and favicon to flicker.

Fix pushState causing stop/reload button and favicon to flicker.

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

[Nate Chapin, 2014-02-13 19:42:11]

sky,

Could I get your opinion on the content/browser/ portion of this patch?

This uses a newly plumbed bit from blink to determine whether the navigation
that just started is within the same document or not and tries not to update the
UI if it's a same document navigation. But...I'm a blink hacker, so I probably
did it wrong :)

Also, what's the preferred way to test this?

[sky, 2014-02-13 22:25:25]

This seems ok to me. Be sure and document to_different_document all the way
through.

As to a test, hopefully you could test what you care about by way of a
TestRenderViewHost or TestWebContents.

https://codereview.chromium.org/161113002/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/161113002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:2574: if
(to_different_document) {
Seems like the delegate/observers should be told the load state has changed
along with |to_different_document|. If they want to ignore the change because
it's within the same document, they can.

[Nate Chapin, 2014-02-27 17:32:04]

On 2014/02/13 22:25:25, sky wrote:
> This seems ok to me. Be sure and document to_different_document all the way
> through.
> 
> As to a test, hopefully you could test what you care about by way of a
> TestRenderViewHost or TestWebContents.
> 
>
https://codereview.chromium.org/161113002/diff/1/content/browser/web_contents...
> File content/browser/web_contents/web_contents_impl.cc (right):
> 
>
https://codereview.chromium.org/161113002/diff/1/content/browser/web_contents...
> content/browser/web_contents/web_contents_impl.cc:2574: if
> (to_different_document) {
> Seems like the delegate/observers should be told the load state has changed
> along with |to_different_document|. If they want to ignore the change because
> it's within the same document, they can.

Sorry for the delay, I got distracted with regressions. I think I've addressed
all comments at this point.

[Nate Chapin, 2014-02-27 17:34:16]

+ygorshenin

I needed to remove a DLOG(INFO) from captive_portal_view.cc in order to satisfy
the presubmit script, and it looks like you were the original author. Is it OK
to remove?

[ygorshenin1, 2014-03-05 09:42:12]

lgtm for chrome/browser/chromeos/*

[Nate Chapin, 2014-03-10 18:01:17]

+creis

Would you mind reviewing the content/ changes?

[Charlie Reis, 2014-03-10 20:07:45]

Wow, thanks for tackling this old bug!  content/ LGTM with a few style nits.

https://codereview.chromium.org/161113002/diff/200001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/161113002/diff/200001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3008: to_different_document,
NULL);
nit: Align with first argument.

https://codereview.chromium.org/161113002/diff/200001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3034: details.get());
nit: align with first argument.

https://codereview.chromium.org/161113002/diff/200001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/161113002/diff/200001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:166:
loadingStateToDifferentDocumentCount_++;
nit: 2 space indent

https://codereview.chromium.org/161113002/diff/200001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:171: return
loadingStateToDifferentDocumentCount_;
nit: 2 space indent

https://codereview.chromium.org/161113002/diff/200001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:413:
base::ASCIIToUTF16("pushState"));
nit: Align with first argument.

https://codereview.chromium.org/161113002/diff/200001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:419: //
LoadingStateChanged should be called 4 times, start and stop for the
Colon rather than comma.

[Nate Chapin, 2014-03-11 19:52:07]

+jschuh, would you mind rubber-stamping the frame_messages.h change?

[jschuh, 2014-03-11 20:28:10]

On 2014/03/11 19:52:07, Nate Chapin wrote:
> +jschuh, would you mind rubber-stamping the frame_messages.h change?

My god, it's full of plumbing.

ipc security lgtm.

[Nate Chapin, 2014-03-11 20:37:31]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-03-11 21:30:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/161113002/210001

[commit-bot: I haz the power, 2014-03-11 22:47:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-11 22:47:06]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Nate Chapin, 2014-03-11 23:00:31]

sky, would you mind take a look at the chrome/ and ui/ changes?

[Nate Chapin, 2014-03-24 21:11:16]

On 2014/03/11 23:00:31, Nate Chapin wrote:
> sky, would you mind take a look at the chrome/ and ui/ changes?

ping @sky?

[sky, 2014-03-25 16:27:40]

LGTM

[Nate Chapin, 2014-03-25 16:30:04]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-03-25 16:30:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/161113002/210001

[commit-bot: I haz the power, 2014-03-25 16:30:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-25 16:30:53]

Failed to apply patch for
chrome/browser/chromeos/login/simple_web_view_dialog.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/login/simple_web_view_dialog.h
  Hunk #1 FAILED at 68.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/chromeos/login/simple_web_view_dialog.h.rej

Patch:       chrome/browser/chromeos/login/simple_web_view_dialog.h
Index: chrome/browser/chromeos/login/simple_web_view_dialog.h
diff --git a/chrome/browser/chromeos/login/simple_web_view_dialog.h
b/chrome/browser/chromeos/login/simple_web_view_dialog.h
index
acfbaa81c4942ef527029514f83e86392d8d75d5..9d8ddd3cb7a3cd621523626d5d2d7ca206a8dc38
100644
--- a/chrome/browser/chromeos/login/simple_web_view_dialog.h
+++ b/chrome/browser/chromeos/login/simple_web_view_dialog.h
@@ -68,7 +68,8 @@ class SimpleWebViewDialog : public views::ButtonListener,
       const content::OpenURLParams& params) OVERRIDE;
 
   // Implements content::WebContentsDelegate:
-  virtual void LoadingStateChanged(content::WebContents* source) OVERRIDE;
+  virtual void LoadingStateChanged(content::WebContents* source,
+                                   bool to_different_document) OVERRIDE;
 
   // Implements LocationBarView::Delegate:
   virtual void NavigationStateChanged(const content::WebContents* source,

[Nate Chapin, 2014-03-25 17:13:16]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-03-25 17:13:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/161113002/280001

[commit-bot: I haz the power, 2014-03-25 18:44:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-25 18:44:09]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[jochen (gone - plz use gerrit), 2014-03-25 19:57:22]

lgtm

[Nate Chapin, 2014-03-25 19:57:42]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-03-25 19:59:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/161113002/470001

[commit-bot: I haz the power, 2014-03-25 20:05:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-25 20:05:26]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[Nate Chapin, 2014-03-25 20:06:29]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-03-25 20:07:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/161113002/470001

[commit-bot: I haz the power, 2014-03-25 21:22:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-25 21:22:57]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[Nate Chapin, 2014-03-25 21:24:34]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-03-25 21:25:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/161113002/470001

[commit-bot: I haz the power, 2014-03-25 22:36:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-25 22:36:36]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[Nate Chapin, 2014-03-27 16:33:40]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-03-27 16:34:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/161113002/760001

[commit-bot: I haz the power, 2014-03-27 16:42:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 16:42:24]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Nate Chapin, 2014-03-27 17:56:00]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-03-27 17:56:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/161113002/760001

[commit-bot: I haz the power, 2014-03-28 00:24:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 00:24:25]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[Nate Chapin, 2014-03-28 15:58:51]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-03-28 16:01:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/161113002/760001

[commit-bot: I haz the power, 2014-03-28 16:06:13]

Change committed as 260143