Add a flag to change when android's progress bar completes

Add a flag to change when android's progress bar completes

The existing behavior is to complete when the top loading frame's load event completes.

This adds 2 alternatives:
* When the main frame's DOMContentLoaded completes
* When the main frame's DOMContentLoaded completes, as well
as any images that started before DOMContentLoaded completes

BUG=513459
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/98e9bd8599c50b3a9342e6109457982435815c4d
Cr-Commit-Position: refs/heads/master@{#402599}

[Nate Chapin, 2016-04-04 20:57:18]

Description was changed from

==========
Stop progress at end of DOMContentLoaded, more or less.

BUG=
==========

to

==========
Stop progress at end of DOMContentLoaded, more or less.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[paulirish, 2016-04-07 21:42:30]

https://codereview.chromium.org/1860743002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/ProgressTracker.cpp (right):

https://codereview.chromium.org/1860743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:153: return type ==
Resource::MainResource || type == Resource::Image || type ==
Resource::CSSStyleSheet || type == Resource::Script || type ==
Resource::XSLStyleSheet;
Mentioned this offline already,

but I think you'll want this to be using the browser definition of
layoutBlocking, as currently this check can misdiagnose async/defer scripts (and
the arriving soon async stylesheets)

https://bugs.chromium.org/p/chromium/issues/detail?id=601593 has more on the
layoutBlocking attribute

[Nate Chapin, 2016-05-06 21:25:10]

Description was changed from

==========
Stop progress at end of DOMContentLoaded, more or less.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add a flag to change when android's progress bar completes

The existing behavior is to complete when the top loading frame's load event
completes.

This adds 2 alternatives:
* When the main frame's DOMContentLoaded completes
* When the main frame's DOMContentLoaded completes, as well as and any images
that started before DOMContentLoaded completes

BUG=513459
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Nate Chapin, 2016-05-06 21:25:29]

Description was changed from

==========
Add a flag to change when android's progress bar completes

The existing behavior is to complete when the top loading frame's load event
completes.

This adds 2 alternatives:
* When the main frame's DOMContentLoaded completes
* When the main frame's DOMContentLoaded completes, as well as and any images
that started before DOMContentLoaded completes

BUG=513459
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add a flag to change when android's progress bar completes

The existing behavior is to complete when the top loading frame's load event
completes.

This adds 2 alternatives:
* When the main frame's DOMContentLoaded completes
* When the main frame's DOMContentLoaded completes, as well
as and any images that started before DOMContentLoaded completes

BUG=513459
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Nate Chapin, 2016-05-06 22:19:07]

japhet@chromium.org changed reviewers:
+ dcheng@chromium.org, ojan@chromium.org

[Nate Chapin, 2016-05-06 22:19:09]

dcheng, is this something you're comforable reviewing?

https://codereview.chromium.org/1860743002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ProgressTracker.h (left):

https://codereview.chromium.org/1860743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ProgressTracker.h:79: long long
m_totalBytesReceived;
Maybe it should be in a separate patch, but I'm not sure it really helps
readability or is important enough for perf to stash these totals. I changed it
to just iterate over m_progressItems and calculate from scratch whenever
necessary, since I found that easier to reason about.

[dcheng, 2016-05-10 06:28:50]

https://codereview.chromium.org/1860743002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/1860743002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:388: } else if (root_ == frame &&
root_->has_started_loading()) {
Maybe just use frame->IsMainFrame()?

https://codereview.chromium.org/1860743002/diff/40001/content/common/content_...
File content/common/content_switches_internal.cc (right):

https://codereview.chromium.org/1860743002/diff/40001/content/common/content_...
content/common/content_switches_internal.cc:88: if (progress_bar_completion ==
"dclAndImages")
My main concern here is that it's not immediately obvious that dcl ==
"DOMContentLoaded"

https://codereview.chromium.org/1860743002/diff/40001/content/public/common/w...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/1860743002/diff/40001/content/public/common/w...
content/public/common/web_preferences.h:58: PROGRESS_BAR_COMPLETION_LOAD_EVENT,
enum class? =)

https://codereview.chromium.org/1860743002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ProgressTracker.cpp (right):

https://codereview.chromium.org/1860743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:209: if
(m_frame->settings()->progressBarCompletion() ==
ProgressBarCompletionDOMContentLoadedAndImage &&
estimatedBytesForPendingRequests == bytesReceived) {
Is exact equality here reliable?

https://codereview.chromium.org/1860743002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebSettings.h (right):

https://codereview.chromium.org/1860743002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebSettings.h:69: enum ProgressBarCompletion {
Ditto

[Nate Chapin, 2016-05-11 00:06:42]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[Nate Chapin, 2016-05-11 00:06:59]

https://codereview.chromium.org/1860743002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/1860743002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:388: } else if (root_ == frame &&
root_->has_started_loading()) {
On 2016/05/10 06:28:50, dcheng wrote:
> Maybe just use frame->IsMainFrame()?

Done.

https://codereview.chromium.org/1860743002/diff/40001/content/common/content_...
File content/common/content_switches_internal.cc (right):

https://codereview.chromium.org/1860743002/diff/40001/content/common/content_...
content/common/content_switches_internal.cc:88: if (progress_bar_completion ==
"dclAndImages")
On 2016/05/10 06:28:50, dcheng wrote:
> My main concern here is that it's not immediately obvious that dcl ==
> "DOMContentLoaded"

I expanded the flag to domContentLoadedAndImages, but left
IDS_FLAGS_PROGRESS_BAR_COMPLETION_DCL_AND_IMAGES, is that OK?

https://codereview.chromium.org/1860743002/diff/40001/content/public/common/w...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/1860743002/diff/40001/content/public/common/w...
content/public/common/web_preferences.h:58: PROGRESS_BAR_COMPLETION_LOAD_EVENT,
On 2016/05/10 06:28:50, dcheng wrote:
> enum class? =)

Done.

https://codereview.chromium.org/1860743002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ProgressTracker.cpp (right):

https://codereview.chromium.org/1860743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:209: if
(m_frame->settings()->progressBarCompletion() ==
ProgressBarCompletionDOMContentLoadedAndImage &&
estimatedBytesForPendingRequests == bytesReceived) {
On 2016/05/10 06:28:50, dcheng wrote:
> Is exact equality here reliable?

I believe so. If at any point incrementProgress() increases bytesReceived such
that it is now greater than estimatedLength, we double estimatedLength (in
theory, this only happens when no Content-Length header is specified). Then,
when a load completes, estimatedLength gets set to bytesReceived, so that we
don't leave it short.

https://codereview.chromium.org/1860743002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebSettings.h (right):

https://codereview.chromium.org/1860743002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebSettings.h:69: enum ProgressBarCompletion {
On 2016/05/10 06:28:50, dcheng wrote:
> Ditto

Done.

[commit-bot: I haz the power, 2016-05-11 00:07:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1860743002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1860743002/60001

[dcheng, 2016-05-11 01:04:25]

https://codereview.chromium.org/1860743002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoaderTypes.h (right):

https://codereview.chromium.org/1860743002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoaderTypes.h:103: enum
ProgressBarCompletion {
Looks like the PS uploaded didn't have the enum class change here.

https://codereview.chromium.org/1860743002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ProgressTracker.cpp (left):

https://codereview.chromium.org/1860743002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:121: reset();
Hmmm, how come we didn't always reset before?

[commit-bot: I haz the power, 2016-05-11 03:59:29]

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

[commit-bot: I haz the power, 2016-05-11 03:59:30]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Nate Chapin, 2016-05-11 22:28:07]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-11 22:28:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1860743002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1860743002/80001

[Nate Chapin, 2016-05-11 22:28:42]

https://codereview.chromium.org/1860743002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoaderTypes.h (right):

https://codereview.chromium.org/1860743002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoaderTypes.h:103: enum
ProgressBarCompletion {
On 2016/05/11 01:04:25, dcheng wrote:
> Looks like the PS uploaded didn't have the enum class change here.

Done.

https://codereview.chromium.org/1860743002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ProgressTracker.cpp (left):

https://codereview.chromium.org/1860743002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:121: reset();
On 2016/05/11 01:04:25, dcheng wrote:
> Hmmm, how come we didn't always reset before?

Legacy, it dates back to this code moving into core/
https://chromium.googlesource.com/chromium/src/+/358faf30aee3eb4474dd9630a230...

[dcheng, 2016-05-11 22:37:08]

Code changes seem good.

Any chance this is testable somehow?

[Nate Chapin, 2016-05-11 22:40:48]

On 2016/05/11 22:37:08, dcheng wrote:
> Code changes seem good.
> 
> Any chance this is testable somehow?

I tested it manually. Given that this code has always been completely untested,
I'm a little scared of what I'll find :)

What layer were you thinking?

[dcheng, 2016-05-11 22:49:34]

On 2016/05/11 at 22:40:48, japhet wrote:
> On 2016/05/11 22:37:08, dcheng wrote:
> > Code changes seem good.
> > 
> > Any chance this is testable somehow?
> 
> I tested it manually. Given that this code has always been completely
untested, I'm a little scared of what I'll find :)
> 
> What layer were you thinking?

I'm not really sure. Is it possible to test at the content layer with the
various settings using FrameLoadWatcher? Or snoop on the progress messages some
other way? I'm not sure how complicated this will be.

[ojan, 2016-05-11 23:48:27]

https://codereview.chromium.org/1860743002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ProgressTracker.cpp (right):

https://codereview.chromium.org/1860743002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:146: static bool
isParserBlockingOrImage(Resource::Type type)
Why do we care about if it's parser blocking?

Also, I wasn't picturing special casing any resource types. I was picturing
special casing loads inside cross-origin frames.

So, I was picturing have two flags:
1. DOMContentLoaded (as you have it)
2. DOMContentLoaded + any same-origin frame (top-level or same origin frame)
resources that were requested before DOMContentLoaded completes. If the frame
level thing is too hard, I think we could probably get away with ignoring
same-origin frames and just look at top-level resource requests.

[Nate Chapin, 2016-05-11 23:58:53]

https://codereview.chromium.org/1860743002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ProgressTracker.cpp (right):

https://codereview.chromium.org/1860743002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:146: static bool
isParserBlockingOrImage(Resource::Type type)
On 2016/05/11 23:48:27, ojan wrote:
> Why do we care about if it's parser blocking?
> 
> Also, I wasn't picturing special casing any resource types. I was picturing
> special casing loads inside cross-origin frames.

I was aiming to implement the first two suggestions in
https://docs.google.com/document/d/1eMrfuUPfokGtjdlLkIZt8hdxNyia_RImQPtWbFYAsx0
, based on chatting w/Kingston. I think it's feasible to do same-origin frames
as well, but I'll need to think a bit more about how to implement that.

The goal with the resource type special-casing was that it wouldn't impact when
the progress bar completed, but that it would be a more accurate way to
calculate the completion percentage while the progress bar was showing. How are
you thinking we should handle, e.g., XHRs started inline?

> 
> So, I was picturing have two flags:
> 1. DOMContentLoaded (as you have it)
> 2. DOMContentLoaded + any same-origin frame (top-level or same origin frame)
> resources that were requested before DOMContentLoaded completes. If the frame
> level thing is too hard, I think we could probably get away with ignoring
> same-origin frames and just look at top-level resource requests.

[commit-bot: I haz the power, 2016-05-12 02:21:12]

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

[commit-bot: I haz the power, 2016-05-12 02:21:13]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[ojan, 2016-05-12 03:43:45]

https://codereview.chromium.org/1860743002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ProgressTracker.cpp (right):

https://codereview.chromium.org/1860743002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:146: static bool
isParserBlockingOrImage(Resource::Type type)
On 2016/05/11 at 23:58:53, Nate Chapin wrote:
> On 2016/05/11 23:48:27, ojan wrote:
> > Why do we care about if it's parser blocking?
> > 
> > Also, I wasn't picturing special casing any resource types. I was picturing
> > special casing loads inside cross-origin frames.
> 
> I was aiming to implement the first two suggestions in
https://docs.google.com/document/d/1eMrfuUPfokGtjdlLkIZt8hdxNyia_RImQPtWbFYAsx0
, based on chatting w/Kingston. I think it's feasible to do same-origin frames
as well, but I'll need to think a bit more about how to implement that.

I think if we can get same-origin frames to work that'd be ideal from a platform
sanity perspective.
 
> The goal with the resource type special-casing was that it wouldn't impact
when the progress bar completed, but that it would be a more accurate way to
calculate the completion percentage while the progress bar was showing. How are
you thinking we should handle, e.g., XHRs started inline?

How are the completion percentage and actually being completed separable?

My thinking here is that we don't have a way of knowing based off resource type
whether the thing being fetched is critical to the user's perception of page
load. For example, modern apps largely fetch all the text for the page in an XHR
started inline, so if we show the page as having been loaded before the XHR is
loaded, we'll be doing so earlier than the user's perception of the page
actually being usable.

> 
> > 
> > So, I was picturing have two flags:
> > 1. DOMContentLoaded (as you have it)
> > 2. DOMContentLoaded + any same-origin frame (top-level or same origin frame)
> > resources that were requested before DOMContentLoaded completes. If the
frame
> > level thing is too hard, I think we could probably get away with ignoring
> > same-origin frames and just look at top-level resource requests.

[Nate Chapin, 2016-06-03 19:07:14]

Ok, I reworked this with a flag to include same origin iframes. I'm not sure
exactly how to handle ad iframes, though: they are often created as about:blank,
then have their content injected via document.write() from the top frame, which
causes them to be treated as same-origin to the top. I tried to work around this
by checking in frame-tree what the last committed url was, which will still be
about:blank in this case.

[ojan, 2016-06-06 21:14:34]

ojan@chromium.org changed reviewers:
+ bmcquade@chromium.org, kinuko@chromium.org

[ojan, 2016-06-06 21:14:35]

l-g-t-m at a high level. I did a reasonable review, but I didn't do a 100%
thorough line-by-line review. Maybe someone from loading team (or dcheng?)
should do so?

[Nate Chapin, 2016-06-06 21:25:41]

On 2016/06/06 21:14:35, ojan wrote:
> l-g-t-m at a high level. I did a reasonable review, but I didn't do a 100%
> thorough line-by-line review. Maybe someone from loading team (or dcheng?)
> should do so?

Yeah, I mostly just wanted your approval of the overall design (esp. the attempt
to not count "same-origin" ad iframes).

dcheng, would you mind taking a more detailed look at this?

[dcheng, 2016-06-08 06:54:17]

Looks good, just a few small questions.

https://codereview.chromium.org/1860743002/diff/100001/content/browser/frame_...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/1860743002/diff/100001/content/browser/frame_...
content/browser/frame_host/frame_tree.cc:378: if (frame->IsMainFrame() &&
frame->has_started_loading())
I wonder if we really need to pass in |frame| here: we could just hardcode
root() here, right? It should still be a no-op if we called this for a non-main
frame, since it won't change the progress for an already loaded main frame.

https://codereview.chromium.org/1860743002/diff/100001/content/common/content...
File content/common/content_switches_internal.h (right):

https://codereview.chromium.org/1860743002/diff/100001/content/common/content...
content/common/content_switches_internal.h:21: ProgressBarCompletionOptions
GetProgressBarCompletionOptions();
Nit: the naming makes it sounds like it can return a bitfield.

https://codereview.chromium.org/1860743002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ProgressTracker.cpp (right):

https://codereview.chromium.org/1860743002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:73: ,
m_progressNotificationTimeInterval(0.1)
Not related to your change, but do you know why these aren't just consts
somewhere in this file? I don't think they're ever written to other than here.

https://codereview.chromium.org/1860743002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:150: if
(m_frame->settings()->progressBarCompletion() !=
ProgressBarCompletion::LoadEvent && m_finishedParsing)
Maybe add a comment here? I guess this is when we loaded something and it
synchronously completed, so we don't need to notify at all?

[kinuko, 2016-06-08 14:26:30]

(Finally took a look...) generally looking good to me too, can we update the
issue description?

https://codereview.chromium.org/1860743002/diff/100001/content/browser/frame_...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/1860743002/diff/100001/content/browser/frame_...
content/browser/frame_host/frame_tree.cc:377: options ==
ProgressBarCompletionOptions::RESOURCES_BEFORE_DCL) {
nit: slightly prefer switch/case than if/else for enums (I see that the other
else case has another option check inside but still in general)

https://codereview.chromium.org/1860743002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ProgressTracker.cpp (right):

https://codereview.chromium.org/1860743002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:28: #include
"core/fetch/Resource.h"
ditto, probably not needed now?

https://codereview.chromium.org/1860743002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:55: ProgressItem(long
long length)
nit: explicit

https://codereview.chromium.org/1860743002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ProgressTracker.h (right):

https://codereview.chromium.org/1860743002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ProgressTracker.h:40: class Resource;
nit: this change's not needed?

https://codereview.chromium.org/1860743002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ProgressTracker.h:47: class CORE_EXPORT
ProgressTracker final : public GarbageCollectedFinalized<ProgressTracker> {
I see that the entire progress bar system is not easily testable but is it
possible / does it make sense to add a simple test for this class? (probably as
a follow-up patch?)

[Nate Chapin, 2016-06-13 17:12:45]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-13 17:13:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1860743002/160001

[commit-bot: I haz the power, 2016-06-13 18:49:50]

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

[commit-bot: I haz the power, 2016-06-13 18:49:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nate Chapin, 2016-06-13 19:58:06]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-13 19:58:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1860743002/160001

[commit-bot: I haz the power, 2016-06-13 21:41:03]

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

[commit-bot: I haz the power, 2016-06-13 21:41:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nate Chapin, 2016-06-14 19:54:20]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-14 19:55:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1860743002/180001

[commit-bot: I haz the power, 2016-06-14 22:24:05]

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

[commit-bot: I haz the power, 2016-06-14 22:24:07]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Nate Chapin, 2016-06-14 22:51:46]

https://codereview.chromium.org/1860743002/diff/100001/content/browser/frame_...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/1860743002/diff/100001/content/browser/frame_...
content/browser/frame_host/frame_tree.cc:377: options ==
ProgressBarCompletionOptions::RESOURCES_BEFORE_DCL) {
On 2016/06/08 14:26:29, kinuko wrote:
> nit: slightly prefer switch/case than if/else for enums (I see that the other
> else case has another option check inside but still in general)

Done.

https://codereview.chromium.org/1860743002/diff/100001/content/browser/frame_...
content/browser/frame_host/frame_tree.cc:378: if (frame->IsMainFrame() &&
frame->has_started_loading())
On 2016/06/08 06:54:17, dcheng wrote:
> I wonder if we really need to pass in |frame| here: we could just hardcode
> root() here, right? It should still be a no-op if we called this for a
non-main
> frame, since it won't change the progress for an already loaded main frame.

True. Done.

https://codereview.chromium.org/1860743002/diff/100001/content/common/content...
File content/common/content_switches_internal.h (right):

https://codereview.chromium.org/1860743002/diff/100001/content/common/content...
content/common/content_switches_internal.h:21: ProgressBarCompletionOptions
GetProgressBarCompletionOptions();
On 2016/06/08 06:54:17, dcheng wrote:
> Nit: the naming makes it sounds like it can return a bitfield.

Renamed to GetProgressBarCompletionPolicy(), and the enum to just
ProgressBarCompletion, since the "Options" wasn't really adding anything IMO.

https://codereview.chromium.org/1860743002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ProgressTracker.cpp (right):

https://codereview.chromium.org/1860743002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:28: #include
"core/fetch/Resource.h"
On 2016/06/08 14:26:30, kinuko wrote:
> ditto, probably not needed now?

Actually, only about half of these includes are necessary. Pruned :)

https://codereview.chromium.org/1860743002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:55: ProgressItem(long
long length)
On 2016/06/08 14:26:30, kinuko wrote:
> nit: explicit

Done.

https://codereview.chromium.org/1860743002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:73: ,
m_progressNotificationTimeInterval(0.1)
On 2016/06/08 06:54:17, dcheng wrote:
> Not related to your change, but do you know why these aren't just consts
> somewhere in this file? I don't think they're ever written to other than here.

Correct, that's probably a WebKit-era configuration option that we never used.
Cleaned up.

https://codereview.chromium.org/1860743002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:150: if
(m_frame->settings()->progressBarCompletion() !=
ProgressBarCompletion::LoadEvent && m_finishedParsing)
On 2016/06/08 06:54:17, dcheng wrote:
> Maybe add a comment here? I guess this is when we loaded something and it
> synchronously completed, so we don't need to notify at all?

Done.

https://codereview.chromium.org/1860743002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ProgressTracker.h (right):

https://codereview.chromium.org/1860743002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ProgressTracker.h:40: class Resource;
On 2016/06/08 14:26:30, kinuko wrote:
> nit: this change's not needed?

Yep, that was necessary in a previous patchset, but not now. Done.

https://codereview.chromium.org/1860743002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ProgressTracker.h:47: class CORE_EXPORT
ProgressTracker final : public GarbageCollectedFinalized<ProgressTracker> {
On 2016/06/08 14:26:30, kinuko wrote:
> I see that the entire progress bar system is not easily testable but is it
> possible / does it make sense to add a simple test for this class? (probably
as
> a follow-up patch?)

Probably :)

[kinuko, 2016-06-16 05:59:03]

Looking good to me, I suspect some of the changes marked 'Done' are not
reflected on PS 10 though?

https://codereview.chromium.org/1860743002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ProgressTracker.cpp (right):

https://codereview.chromium.org/1860743002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:150: if
(m_frame->settings()->progressBarCompletion() !=
ProgressBarCompletion::LoadEvent && m_finishedParsing)
On 2016/06/14 22:51:46, Nate Chapin wrote:
> On 2016/06/08 06:54:17, dcheng wrote:
> > Maybe add a comment here? I guess this is when we loaded something and it
> > synchronously completed, so we don't need to notify at all?
> 
> Done.

The comment's not reflected on the newest patch yet?

https://codereview.chromium.org/1860743002/diff/180001/content/public/common/...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/1860743002/diff/180001/content/public/common/...
content/public/common/web_preferences.h:57: enum class ProgressBarCompletion {
nit: add a comment?

https://codereview.chromium.org/1860743002/diff/180001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebSettings.h (right):

https://codereview.chromium.org/1860743002/diff/180001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebSettings.h:76: enum class ProgressBarCompletion
{
Need to define so many same enums (and static asserts) makes me feel sad :(

[Nate Chapin, 2016-06-16 20:58:51]

https://codereview.chromium.org/1860743002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ProgressTracker.cpp (right):

https://codereview.chromium.org/1860743002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:150: if
(m_frame->settings()->progressBarCompletion() !=
ProgressBarCompletion::LoadEvent && m_finishedParsing)
On 2016/06/16 05:59:03, kinuko wrote:
> On 2016/06/14 22:51:46, Nate Chapin wrote:
> > On 2016/06/08 06:54:17, dcheng wrote:
> > > Maybe add a comment here? I guess this is when we loaded something and it
> > > synchronously completed, so we don't need to notify at all?
> > 
> > Done.
> 
> The comment's not reflected on the newest patch yet?

Huh, done for real.

https://codereview.chromium.org/1860743002/diff/180001/content/public/common/...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/1860743002/diff/180001/content/public/common/...
content/public/common/web_preferences.h:57: enum class ProgressBarCompletion {
On 2016/06/16 05:59:03, kinuko wrote:
> nit: add a comment?

Done.

https://codereview.chromium.org/1860743002/diff/180001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebSettings.h (right):

https://codereview.chromium.org/1860743002/diff/180001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebSettings.h:76: enum class ProgressBarCompletion
{
On 2016/06/16 05:59:03, kinuko wrote:
> Need to define so many same enums (and static asserts) makes me feel sad :(

Yeah...it'd be awesome if we could get a general solution to minimizing mirrored
enums.

[kinuko, 2016-06-17 04:34:50]

lgtm

https://codereview.chromium.org/1860743002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ProgressTracker.cpp (right):

https://codereview.chromium.org/1860743002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:73: ,
m_progressNotificationTimeInterval(0.1)
...and we could probably clean-up these with constants?

[dcheng, 2016-06-17 20:53:06]

LGTM as well, with a few small comments.

https://codereview.chromium.org/1860743002/diff/180001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebSettings.h (right):

https://codereview.chromium.org/1860743002/diff/180001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebSettings.h:76: enum class ProgressBarCompletion
{
On 2016/06/16 20:58:51, Nate Chapin wrote:
> On 2016/06/16 05:59:03, kinuko wrote:
> > Need to define so many same enums (and static asserts) makes me feel sad :(
> 
> Yeah...it'd be awesome if we could get a general solution to minimizing
mirrored
> enums.

Maybe let's see if the chrome owners think it makes sense to use the public
Blink enums in about_flags.cc. It kind of makes sense to me if we're tweaking
Blink config values?

(Not necessary to do this in this patch though)

https://codereview.chromium.org/1860743002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ProgressTracker.cpp (right):

https://codereview.chromium.org/1860743002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:73: ,
m_progressNotificationTimeInterval(0.1)
On 2016/06/17 04:34:50, kinuko wrote:
> ...and we could probably clean-up these with constants?

I think I asked about this as well in a previous patchset, maybe it just didn't
get uploaded in the latest patchset?

https://codereview.chromium.org/1860743002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:141: m_progressValue =
1;
This is probably a dumb question, but why can't we just check that
m_progressValue < 1 instead of using a separate bool?

[Nate Chapin, 2016-06-17 22:25:32]

https://codereview.chromium.org/1860743002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ProgressTracker.cpp (right):

https://codereview.chromium.org/1860743002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:73: ,
m_progressNotificationTimeInterval(0.1)
On 2016/06/17 20:53:06, dcheng wrote:
> On 2016/06/17 04:34:50, kinuko wrote:
> > ...and we could probably clean-up these with constants?
> 
> I think I asked about this as well in a previous patchset, maybe it just
didn't
> get uploaded in the latest patchset?

I remember making those changes, I apparently accidnentally nuked them before
upload? :-/

https://codereview.chromium.org/1860743002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:141: m_progressValue =
1;
On 2016/06/17 20:53:06, dcheng wrote:
> This is probably a dumb question, but why can't we just check that
> m_progressValue < 1 instead of using a separate bool?

It's the way it's always been!

Done. :)

[Nate Chapin, 2016-06-17 22:39:36]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2016-06-17 22:39:36]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1860743002/#ps220001
(title: "Remove unneeded variables on ProgressTracker")

[commit-bot: I haz the power, 2016-06-17 22:39:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1860743002/220001

[commit-bot: I haz the power, 2016-06-17 22:48:31]

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

[commit-bot: I haz the power, 2016-06-17 22:48:32]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Nate Chapin, 2016-06-20 17:05:40]

japhet@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Nate Chapin, 2016-06-20 17:05:41]

tedchoc, do these changes to ToolbarManager.java look ok? Some tweaks were
required to handle the possibility of finishing the progress bar earlier in the
document lifetime: if it finished before didStopLoading() and a new navigation
started between progress bar completion and didStopLoading(), the progress bar
would never reappear :/

[Ted C, 2016-06-20 18:06:02]

tedchoc@chromium.org changed reviewers:
+ mdjones@chromium.org

[Ted C, 2016-06-20 18:06:03]

Adding mdjones@ as he's looked at progress a bunch recently

But I will say that I'm a bit confused.  We start load progress in
onDidStartProvisionalLoadForFrame.  I don't see how having the progress bar end
earlier in the load lifecycle should affect that.

The onLoadStopped signal from the previous page "should" happen before
onDidStartProvisionalLoadForFrame for the new one right?  If not, then it seems
like we'd have this bug already.

Right now, start and stop of the load progress are loosely expected to be called
just once per page load.  We should also make sure with these experiments that
we're not exposing other previously somewhat working stuff.

But I'll stop rambling and have mdjones@ jump in.

[mdjones, 2016-06-20 20:58:41]

https://codereview.chromium.org/1860743002/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/1860743002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:321:
updateLoadProgress(tab.getProgress());
If I'm not mistaken, this should always happen after
onDidStartProvisionalLoadForFrame. Did something change where this wouldn't be
the case?

https://codereview.chromium.org/1860743002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:1154:
if (progress == 100) finishLoadProgress(true);
Can updateLoadProgress hit 100 without triggering onLoadStopped?

[Nate Chapin, 2016-06-21 21:06:29]

Ok, I double checked, and one of those two changes to ToolbarManager isn't
necessary. The change in updateLoadProgress() still looks necessary, though.

https://codereview.chromium.org/1860743002/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/1860743002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:321:
updateLoadProgress(tab.getProgress());
On 2016/06/20 20:58:40, mdjones wrote:
> If I'm not mistaken, this should always happen after
> onDidStartProvisionalLoadForFrame. Did something change where this wouldn't be
> the case?

I think there may have been a bug fix between when I originally made this change
and now. I can't reproduce the "progress bar never reappears" issue I was
having. Dropped this change.

https://codereview.chromium.org/1860743002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:1154:
if (progress == 100) finishLoadProgress(true);
On 2016/06/20 20:58:41, mdjones wrote:
> Can updateLoadProgress hit 100 without triggering onLoadStopped?

On trunk, no, 100 is only triggered as part of onLoadStopped. With this change,
the experimental flags allow the progress bar to complete before onLoadStopped.
onLoadStopped will still be called eventually, but there could easily be a delay
of several seconds between the progress bar completing and onLoadStopped..

[mdjones, 2016-06-21 23:58:17]

lgtm

[Ted C, 2016-06-22 00:01:19]

On 2016/06/21 23:58:17, mdjones wrote:
> lgtm

rs lgtm

[Nate Chapin, 2016-06-22 23:18:45]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2016-06-22 23:18:46]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1860743002/#ps240001
(title: "Drop some unnecessary changes to ToolbarManagar.java")

[commit-bot: I haz the power, 2016-06-22 23:19:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1860743002/240001

[commit-bot: I haz the power, 2016-06-23 01:32:43]

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

[commit-bot: I haz the power, 2016-06-23 01:32:46]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Nate Chapin, 2016-06-23 18:40:48]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2016-06-23 18:41:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1860743002/240001

[commit-bot: I haz the power, 2016-06-23 20:23:48]

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

[commit-bot: I haz the power, 2016-06-23 20:23:50]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Nate Chapin, 2016-06-24 21:26:44]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-24 21:27:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nate Chapin, 2016-06-24 21:28:03]

japhet@chromium.org changed reviewers:
+ jwd@chromium.org

[Nate Chapin, 2016-06-24 21:28:22]

jwd, would you mind reviewing the histograms.xml change?

[commit-bot: I haz the power, 2016-06-24 22:59:14]

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

[commit-bot: I haz the power, 2016-06-24 22:59:16]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Nate Chapin, 2016-06-27 21:45:22]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2016-06-27 21:45:23]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
dcheng@chromium.org, tedchoc@chromium.org, mdjones@chromium.org
Link to the patchset: https://codereview.chromium.org/1860743002/#ps260001
(title: "Add missing histograms.xml entry")

[Nate Chapin, 2016-06-27 21:45:32]

The CQ bit was unchecked by japhet@chromium.org

[Nate Chapin, 2016-06-27 21:45:43]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-27 21:47:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-27 23:20:43]

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

[commit-bot: I haz the power, 2016-06-27 23:20:45]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jwd, 2016-06-28 15:45:07]

LGTM with tiny nit:
in the 2nd alternative "...as well as and any images..." extra "and".

[Nate Chapin, 2016-06-28 18:36:34]

Description was changed from

==========
Add a flag to change when android's progress bar completes

The existing behavior is to complete when the top loading frame's load event
completes.

This adds 2 alternatives:
* When the main frame's DOMContentLoaded completes
* When the main frame's DOMContentLoaded completes, as well
as and any images that started before DOMContentLoaded completes

BUG=513459
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add a flag to change when android's progress bar completes

The existing behavior is to complete when the top loading frame's load event
completes.

This adds 2 alternatives:
* When the main frame's DOMContentLoaded completes
* When the main frame's DOMContentLoaded completes, as well
as any images that started before DOMContentLoaded completes

BUG=513459
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Nate Chapin, 2016-06-28 19:00:03]

On 2016/06/28 15:45:07, jwd wrote:
> LGTM with tiny nit:
> in the 2nd alternative "...as well as and any images..." extra "and".

Done, thanks.

[Nate Chapin, 2016-06-28 19:02:17]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2016-06-28 19:02:18]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
dcheng@chromium.org, tedchoc@chromium.org, jwd@chromium.org,
mdjones@chromium.org
Link to the patchset: https://codereview.chromium.org/1860743002/#ps280001
(title: "Add missing flag default")

[commit-bot: I haz the power, 2016-06-28 19:04:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 19:53:44]

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

[commit-bot: I haz the power, 2016-06-28 19:53:47]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nate Chapin, 2016-06-28 19:55:48]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2016-06-28 19:57:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 20:38:18]

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

[commit-bot: I haz the power, 2016-06-28 20:38:21]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nate Chapin, 2016-06-28 22:12:32]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2016-06-28 22:14:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 23:49:18]

Description was changed from

==========
Add a flag to change when android's progress bar completes

The existing behavior is to complete when the top loading frame's load event
completes.

This adds 2 alternatives:
* When the main frame's DOMContentLoaded completes
* When the main frame's DOMContentLoaded completes, as well
as any images that started before DOMContentLoaded completes

BUG=513459
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add a flag to change when android's progress bar completes

The existing behavior is to complete when the top loading frame's load event
completes.

This adds 2 alternatives:
* When the main frame's DOMContentLoaded completes
* When the main frame's DOMContentLoaded completes, as well
as any images that started before DOMContentLoaded completes

BUG=513459
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-06-28 23:49:21]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-06-28 23:52:44]

Description was changed from

==========
Add a flag to change when android's progress bar completes

The existing behavior is to complete when the top loading frame's load event
completes.

This adds 2 alternatives:
* When the main frame's DOMContentLoaded completes
* When the main frame's DOMContentLoaded completes, as well
as any images that started before DOMContentLoaded completes

BUG=513459
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add a flag to change when android's progress bar completes

The existing behavior is to complete when the top loading frame's load event
completes.

This adds 2 alternatives:
* When the main frame's DOMContentLoaded completes
* When the main frame's DOMContentLoaded completes, as well
as any images that started before DOMContentLoaded completes

BUG=513459
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/98e9bd8599c50b3a9342e6109457982435815c4d
Cr-Commit-Position: refs/heads/master@{#402599}
==========

[commit-bot: I haz the power, 2016-06-28 23:52:46]

Patchset 15 (id:??) landed as
https://crrev.com/98e9bd8599c50b3a9342e6109457982435815c4d
Cr-Commit-Position: refs/heads/master@{#402599}