reflow comments in modules/[fetch,indexeddb]

reflow comments in modules/[fetch,indexeddb]

BUG=563793
NOTRY=true

Committed: https://crrev.com/3ef2412e118b0018ccb7291358391131894c4e23
Cr-Commit-Position: refs/heads/master@{#423151}

[Nico, 2016-10-04 19:08:26]

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

[Nico, 2016-10-04 19:08:47]

thakis@chromium.org changed reviewers:
+ dcheng@chromium.org, haraken@chromium.org

[Nico, 2016-10-04 19:08:48]

dcheng: review
haraken: owners

[commit-bot: I haz the power, 2016-10-04 19:09:03]

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

[jsbell, 2016-10-04 20:10:30]

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2016-10-04 20:10:30]

indexeddb lgtm

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/filesystem/DOMWindowFileSystem.h (right):

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/filesystem/DOMWindowFileSystem.h:54: // They
are placed here and in all capital letters so they can be checked
This comment needs updating... or just remove it. We have static_asserts.

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/gamepad/NavigatorGamepad.cpp (right):

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/gamepad/NavigatorGamepad.cpp:250: // became
hidden.  We create a new snapshot and compare them.
Two spaces here is unexpected - intentional style choice, edge case in the tool,
...?

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/indexeddb/IDBRequestTest.cpp (right):

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBRequestTest.cpp:84: // clear up
any intermediaries.  Ensure an assertion is not raised.
An other double space after '.'

[Nico, 2016-10-04 20:16:01]

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/filesystem/DOMWindowFileSystem.h (right):

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/filesystem/DOMWindowFileSystem.h:54: // They
are placed here and in all capital letters so they can be checked
On 2016/10/04 20:10:30, jsbell wrote:
> This comment needs updating... or just remove it. We have static_asserts.

The comment sounds like "these are here for static_asserts" to me. I don't know
what to replace the comment with.

If it's ok, I'll just keep this as-is and you can change it to what you want
(tbr=me) later.

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/gamepad/NavigatorGamepad.cpp (right):

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/gamepad/NavigatorGamepad.cpp:250: // became
hidden.  We create a new snapshot and compare them.
On 2016/10/04 20:10:30, jsbell wrote:
> Two spaces here is unexpected - intentional style choice, edge case in the
tool,
> ...?

I use vim's gq to join lines manually, and it defaults to two spaces after
periods.

Looks like we deliberately decided that we don't care about this a while ago:
https://codereview.chromium.org/609633004

[Nico, 2016-10-04 20:16:06]

The CQ bit was unchecked by thakis@chromium.org

[Nico, 2016-10-04 20:16:18]

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

[commit-bot: I haz the power, 2016-10-04 20:16:54]

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-10-04 20:59:57]

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

[commit-bot: I haz the power, 2016-10-04 20:59:58]

Dry run: This issue passed the CQ dry run.

[Nico, 2016-10-04 22:15:25]

On 2016/10/04 20:16:01, Nico wrote:
>
https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/filesystem/DOMWindowFileSystem.h
(right):
> 
>
https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/filesystem/DOMWindowFileSystem.h:54: // They
> are placed here and in all capital letters so they can be checked
> On 2016/10/04 20:10:30, jsbell wrote:
> > This comment needs updating... or just remove it. We have static_asserts.
> 
> The comment sounds like "these are here for static_asserts" to me. I don't
know
> what to replace the comment with.
> 
> If it's ok, I'll just keep this as-is and you can change it to what you want
> (tbr=me) later.
> 
>
https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/gamepad/NavigatorGamepad.cpp (right):
> 
>
https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/gamepad/NavigatorGamepad.cpp:250: // became
> hidden.  We create a new snapshot and compare them.
> On 2016/10/04 20:10:30, jsbell wrote:
> > Two spaces here is unexpected - intentional style choice, edge case in the
> tool,
> > ...?
> 
> I use vim's gq to join lines manually, and it defaults to two spaces after
> periods.
> 
> Looks like we deliberately decided that we don't care about this a while ago:
> https://codereview.chromium.org/609633004

dcheng: ping

[dcheng, 2016-10-04 23:03:04]

LGTM with some random thoughts

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp (right):

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp:318: //        means
the callback will be on the original context openCursor was
Nit: FIXMEs aren't usually indented like this

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/indexeddb/IDBEventDispatcher.h (right):

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBEventDispatcher.h:48:
HeapVector<Member<EventTarget>>&);  // The target first and then its
Nit: my usual thoughts about inline comments on member definitions (e.g. own
line)

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right):

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:333: // FIXME: In
an ideal world, we should return true as long as anyone has a or
Nit: same comment about FIXME; alignment

[haraken, 2016-10-05 01:11:40]

LGTM with the comments addressed.

[Nico, 2016-10-05 01:33:17]

Thanks!

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp (right):

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp:318: //        means
the callback will be on the original context openCursor was
On 2016/10/04 23:03:04, dcheng wrote:
> Nit: FIXMEs aren't usually indented like this

Done.

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/indexeddb/IDBEventDispatcher.h (right):

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBEventDispatcher.h:48:
HeapVector<Member<EventTarget>>&);  // The target first and then its
On 2016/10/04 23:03:04, dcheng wrote:
> Nit: my usual thoughts about inline comments on member definitions (e.g. own
> line)

Done. (It's not a member but a function here though)

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right):

https://codereview.chromium.org/2388423003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:333: // FIXME: In
an ideal world, we should return true as long as anyone has a or
On 2016/10/04 23:03:04, dcheng wrote:
> Nit: same comment about FIXME; alignment

Done.

[Nico, 2016-10-05 01:33:22]

The CQ bit was checked by thakis@chromium.org

[Nico, 2016-10-05 01:33:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jsbell@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2388423003/#ps20001
(title: "comments")

[commit-bot: I haz the power, 2016-10-05 01:33:44]

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-10-05 03:02:21]

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

[commit-bot: I haz the power, 2016-10-05 03:02:22]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Nico, 2016-10-05 13:44:05]

Description was changed from

==========
reflow comments in modules/[fetch,indexeddb]

BUG=563793
==========

to

==========
reflow comments in modules/[fetch,indexeddb]

BUG=563793
NOTRY=true
==========

[Nico, 2016-10-05 13:44:10]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2016-10-05 13:44:27]

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-10-05 13:49:31]

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

[commit-bot: I haz the power, 2016-10-05 13:49:32]

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

[Nico, 2016-10-05 14:02:54]

The CQ bit was checked by thakis@chromium.org

[Nico, 2016-10-05 14:02:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jsbell@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2388423003/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-05 14:03:08]

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-10-05 14:08:53]

Description was changed from

==========
reflow comments in modules/[fetch,indexeddb]

BUG=563793
NOTRY=true
==========

to

==========
reflow comments in modules/[fetch,indexeddb]

BUG=563793
NOTRY=true
==========

[commit-bot: I haz the power, 2016-10-05 14:08:55]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-05 14:12:23]

Description was changed from

==========
reflow comments in modules/[fetch,indexeddb]

BUG=563793
NOTRY=true
==========

to

==========
reflow comments in modules/[fetch,indexeddb]

BUG=563793
NOTRY=true

Committed: https://crrev.com/3ef2412e118b0018ccb7291358391131894c4e23
Cr-Commit-Position: refs/heads/master@{#423151}
==========

[commit-bot: I haz the power, 2016-10-05 14:12:24]

Patchset 3 (id:??) landed as
https://crrev.com/3ef2412e118b0018ccb7291358391131894c4e23
Cr-Commit-Position: refs/heads/master@{#423151}