Move |is_static_data| check down in DetermineRevalidationPolicy

Move |is_static_data| check down in DetermineRevalidationPolicy

This CL moves

 if (is_static_data)
   return kUse;

section down in DetermineRevalidationPolicy as a preparation to factor out some
|return kReload;| statements.

BUG=652228, 714574
R=hiroshige@chromium.org

Review-Url: https://codereview.chromium.org/2829043002
Cr-Commit-Position: refs/heads/master@{#466931}
Committed: https://chromium.googlesource.com/chromium/src/+/58197789bfafee408d8fc728c0ed2f1da81bbbed

[yhirano, 2017-04-20 09:01:46]

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

[commit-bot: I haz the power, 2017-04-20 09:02:35]

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

[hiroshige, 2017-04-20 09:33:21]

Could you explain the intention of this?
Is there specific case where we want to return kReload for static data?

[commit-bot: I haz the power, 2017-04-20 10:44:51]

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

[commit-bot: I haz the power, 2017-04-20 10:44:52]

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

[yhirano, 2017-04-20 11:05:07]

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

[commit-bot: I haz the power, 2017-04-20 11:05:28]

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

[yhirano, 2017-04-20 11:12:54]

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

[commit-bot: I haz the power, 2017-04-20 11:13:04]

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

[yhirano, 2017-04-20 11:16:20]

Description was changed from

==========
Give is_static_data a lower priority in DetermineRevalidationPolicy

BUG=652228
R=hiroshige@chromium.org, japhet@chromium.org
==========

to

==========
Move |is_static_data| check down in DetermineRevalidationPolicy

This CL moves

 if (is_static_data)
   return kUse;

section down in DetermineRevalidationPolicy as a preparation to factor out some
|return kReload;| statements.

BUG=652228
R=hiroshige@chromium.org, japhet@chromium.org
==========

[yhirano, 2017-04-20 11:16:20]

yhirano@chromium.org changed reviewers:
- japhet@chromium.org

[yhirano, 2017-04-20 11:16:51]

Description was changed from

==========
Move |is_static_data| check down in DetermineRevalidationPolicy

This CL moves

 if (is_static_data)
   return kUse;

section down in DetermineRevalidationPolicy as a preparation to factor out some
|return kReload;| statements.

BUG=652228
R=hiroshige@chromium.org, japhet@chromium.org
==========

to

==========
Move |is_static_data| check down in DetermineRevalidationPolicy

This CL moves

 if (is_static_data)
   return kUse;

section down in DetermineRevalidationPolicy as a preparation to factor out some
|return kReload;| statements.

BUG=652228
R=hiroshige@chromium.org
==========

[yhirano, 2017-04-20 11:19:09]

On 2017/04/20 09:33:21, hiroshige wrote:
> Could you explain the intention of this?
> Is there specific case where we want to return kReload for static data?

I didn't think you were awoke :)

Done. Initially I thought I didn't need |!is_static_data &&|, but it was wrong
according to a test failure.

The motivation is that I will factor some kReload cases out in a subsequent CL
and so I don't want to have a kUse statement in between.

[commit-bot: I haz the power, 2017-04-20 13:06:44]

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

[commit-bot: I haz the power, 2017-04-20 13:06:45]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-04-20 19:24:39]

On 2017/04/20 11:19:09, yhirano wrote:
> On 2017/04/20 09:33:21, hiroshige wrote:
> > Could you explain the intention of this?
> > Is there specific case where we want to return kReload for static data?
> 
> I didn't think you were awoke :)
> 
> Done. Initially I thought I didn't need |!is_static_data &&|, but it was wrong
> according to a test failure.
> 
> The motivation is that I will factor some kReload cases out in a subsequent CL
> and so I don't want to have a kUse statement in between.

lgtm.

I also suspect additional |is_static_data| is not really needed (especially the
one related to revalidation/304), but we can leave it for separate investigation
because it isn't related to your context.

Also, this looks consistent with Nate's plan to group the conditions of
DetermineRevalidationPolicy(), e.g. high-priority kReloads, mid-priority kUse,
low-priority kReloads, and then kRevalidate/kUse or something like.
I think we should have a tracking bug for that and add it to BUG= line here.
Nate, do you have an existing tracking bug for reordering/refactoring
DetermineRevalidationPolicy(), or otherwise could you create a new one?\

[hiroshige, 2017-04-20 23:27:05]

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

[hiroshige, 2017-04-20 23:27:06]

Oh, adding Nate.

[yhirano, 2017-04-21 04:08:41]

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

[commit-bot: I haz the power, 2017-04-21 04:09:05]

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, 2017-04-21 05:25:54]

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

[commit-bot: I haz the power, 2017-04-21 05:25:54]

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

[Nate Chapin, 2017-04-21 18:21:18]

No, I don't have an existing tracking bug for DetermineRevalidationPolicy
cleanup. This LGTM.

[yhirano, 2017-04-24 10:11:25]

Description was changed from

==========
Move |is_static_data| check down in DetermineRevalidationPolicy

This CL moves

 if (is_static_data)
   return kUse;

section down in DetermineRevalidationPolicy as a preparation to factor out some
|return kReload;| statements.

BUG=652228
R=hiroshige@chromium.org
==========

to

==========
Move |is_static_data| check down in DetermineRevalidationPolicy

This CL moves

 if (is_static_data)
   return kUse;

section down in DetermineRevalidationPolicy as a preparation to factor out some
|return kReload;| statements.

BUG=652228, 714574
R=hiroshige@chromium.org
==========

[yhirano, 2017-04-24 10:11:52]

Thank you.

I filed https://crbug.com/714574.

[yhirano, 2017-04-25 06:01:48]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2017-04-25 06:01:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org
Link to the patchset: https://codereview.chromium.org/2829043002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2017-04-25 06:02:17]

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, 2017-04-25 08:02:39]

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

[commit-bot: I haz the power, 2017-04-25 08:02:40]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yhirano, 2017-04-25 08:03:43]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2017-04-25 08:03:50]

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, 2017-04-25 09:03:43]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493107423225460,
"parent_rev": "0363d88c658806d011c31a4f48f25779e9bc5f22", "commit_rev":
"58197789bfafee408d8fc728c0ed2f1da81bbbed"}

[commit-bot: I haz the power, 2017-04-25 09:03:55]

Description was changed from

==========
Move |is_static_data| check down in DetermineRevalidationPolicy

This CL moves

 if (is_static_data)
   return kUse;

section down in DetermineRevalidationPolicy as a preparation to factor out some
|return kReload;| statements.

BUG=652228, 714574
R=hiroshige@chromium.org
==========

to

==========
Move |is_static_data| check down in DetermineRevalidationPolicy

This CL moves

 if (is_static_data)
   return kUse;

section down in DetermineRevalidationPolicy as a preparation to factor out some
|return kReload;| statements.

BUG=652228, 714574
R=hiroshige@chromium.org

Review-Url: https://codereview.chromium.org/2829043002
Cr-Commit-Position: refs/heads/master@{#466931}
Committed:
https://chromium.googlesource.com/chromium/src/+/58197789bfafee408d8fc728c0ed...
==========

[commit-bot: I haz the power, 2017-04-25 09:03:56]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/58197789bfafee408d8fc728c0ed...