bluetooth: Change BluetoothSupplement::from(ScriptState*) to from(LocalFrame*)

bluetooth: Change BluetoothSupplement::from(ScriptState*) to from(LocalFrame*)

BluetoothSupplement belongs to LocalFrame so we should only have
from(LocalFrame*).

Also removes bluetooth from Platform as it is not needed yet.

BUG=420275
Committed: https://crrev.com/bc969e58f6459e8552e2b0d128f030673f7c7a54
Cr-Commit-Position: refs/heads/master@{#354070}

[ortuno, 2015-10-09 17:33:00]

ortuno@chromium.org changed reviewers:
+ haraken@chromium.org

[ortuno, 2015-10-09 17:33:01]

haraken: PTAL

[ortuno, 2015-10-09 17:33:04]

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

[commit-bot: I haz the power, 2015-10-09 17:35:02]

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

[commit-bot: I haz the power, 2015-10-09 19:01:35]

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

[commit-bot: I haz the power, 2015-10-09 19:01:36]

Dry run: This issue passed the CQ dry run.

[haraken, 2015-10-10 14:09:57]

https://codereview.chromium.org/1399623003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp
(right):

https://codereview.chromium.org/1399623003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp:31: if
(supplement && supplement->m_bluetooth)

If the supplement is 0, don't you need to create a new BluetoothSupplement and
provide it to the frame?

c.f., A common programming pattern of Supplement::from is like this:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1399623003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp:36:
WebBluetooth* BluetoothSupplement::getFromScriptState(ScriptState* scriptState)

getFromScriptState => fromScriptState (Blink doesn't use a "get" prefix).

https://codereview.chromium.org/1399623003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp:39: if
(window && window->frame()) {

It is unsafe to use window->frame() directly. You need to use 
toFrameIfNotDetached() in V8Binding.h.

[ortuno, 2015-10-12 17:33:52]

Thanks!

https://codereview.chromium.org/1399623003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp
(right):

https://codereview.chromium.org/1399623003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp:31: if
(supplement && supplement->m_bluetooth)
On 2015/10/10 at 14:09:57, haraken wrote:
> If the supplement is 0, don't you need to create a new BluetoothSupplement and
provide it to the frame?
> 
> c.f., A common programming pattern of Supplement::from is like this:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

We provide the Supplement to the frame here:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

So we should always find the Supplement. The other classes behind a flag in that
function follow the same pattern:


https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Changed the code to make it clearer.

https://codereview.chromium.org/1399623003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp:36:
WebBluetooth* BluetoothSupplement::getFromScriptState(ScriptState* scriptState)
On 2015/10/10 at 14:09:57, haraken wrote:
> getFromScriptState => fromScriptState (Blink doesn't use a "get" prefix).

Done.

https://codereview.chromium.org/1399623003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp:39: if
(window && window->frame()) {
On 2015/10/10 at 14:09:57, haraken wrote:
> It is unsafe to use window->frame() directly. You need to use 
toFrameIfNotDetached() in V8Binding.h.

As suggested in the other CL I changed it to get the frame from the
executionContext.

Why do we need to check !executionContext->isDocument()?

[ortuno, 2015-10-12 17:33:58]

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

[commit-bot: I haz the power, 2015-10-12 17:36:59]

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

[commit-bot: I haz the power, 2015-10-12 19:00:57]

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

[commit-bot: I haz the power, 2015-10-12 19:01:01]

Dry run: This issue passed the CQ dry run.

[Jeffrey Yasskin, 2015-10-12 21:57:24]

https://codereview.chromium.org/1399623003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp
(right):

https://codereview.chromium.org/1399623003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp:39: if
(window && window->frame()) {
On 2015/10/12 17:33:52, ortuno wrote:
> On 2015/10/10 at 14:09:57, haraken wrote:
> > It is unsafe to use window->frame() directly. You need to use 
> toFrameIfNotDetached() in V8Binding.h.

Thanks Kentaro for catching my mistakes in writing this class. Was there any way
for me to discover that window->frame() was unsafe? e.g. neither
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...
nor
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...
has a comment saying anything's unsafe.

> Why do we need to check !executionContext->isDocument()?

The static_cast<Document*>(executionContext) is only valid if executionContext
is a Document.

[haraken, 2015-10-12 23:29:25]

https://codereview.chromium.org/1399623003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp
(right):

https://codereview.chromium.org/1399623003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp:39: if
(window && window->frame()) {
On 2015/10/12 21:57:24, Jeffrey Yasskin wrote:
> On 2015/10/12 17:33:52, ortuno wrote:
> > On 2015/10/10 at 14:09:57, haraken wrote:
> > > It is unsafe to use window->frame() directly. You need to use 
> > toFrameIfNotDetached() in V8Binding.h.
> 
> Thanks Kentaro for catching my mistakes in writing this class. Was there any
way
> for me to discover that window->frame() was unsafe? e.g. neither
>
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...
> nor
>
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...
> has a comment saying anything's unsafe.
> 
> > Why do we need to check !executionContext->isDocument()?
> 
> The static_cast<Document*>(executionContext) is only valid if executionContext
> is a Document.

Sorry for not having a comment about it. We had to check
isCurrentlyDisplayedInFrame() before accessing window->frame() (c.f.,
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...),
but dcheng@ recently removed the restriction. (So actually it's now safe to
access window->frame().)

Sorry about the confusion. I'll clean up the code and add a good comment.

[ortuno, 2015-10-13 01:42:20]

On 2015/10/12 23:29:25, haraken wrote:
>
https://codereview.chromium.org/1399623003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp
> (right):
> 
>
https://codereview.chromium.org/1399623003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp:39: if
> (window && window->frame()) {
> On 2015/10/12 21:57:24, Jeffrey Yasskin wrote:
> > On 2015/10/12 17:33:52, ortuno wrote:
> > > On 2015/10/10 at 14:09:57, haraken wrote:
> > > > It is unsafe to use window->frame() directly. You need to use 
> > > toFrameIfNotDetached() in V8Binding.h.
> > 
> > Thanks Kentaro for catching my mistakes in writing this class. Was there any
> way
> > for me to discover that window->frame() was unsafe? e.g. neither
> >
>
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...
> > nor
> >
>
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...
> > has a comment saying anything's unsafe.
> > 
> > > Why do we need to check !executionContext->isDocument()?
> > 
> > The static_cast<Document*>(executionContext) is only valid if
executionContext
> > is a Document.
> 
> Sorry for not having a comment about it. We had to check
> isCurrentlyDisplayedInFrame() before accessing window->frame() (c.f.,
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...),
> but dcheng@ recently removed the restriction. (So actually it's now safe to
> access window->frame().)
> 
> Sorry about the confusion. I'll clean up the code and add a good comment.

Sooo LGTM?

[haraken, 2015-10-13 01:44:34]

On 2015/10/13 01:42:20, ortuno wrote:
> On 2015/10/12 23:29:25, haraken wrote:
> >
>
https://codereview.chromium.org/1399623003/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/1399623003/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp:39: if
> > (window && window->frame()) {
> > On 2015/10/12 21:57:24, Jeffrey Yasskin wrote:
> > > On 2015/10/12 17:33:52, ortuno wrote:
> > > > On 2015/10/10 at 14:09:57, haraken wrote:
> > > > > It is unsafe to use window->frame() directly. You need to use 
> > > > toFrameIfNotDetached() in V8Binding.h.
> > > 
> > > Thanks Kentaro for catching my mistakes in writing this class. Was there
any
> > way
> > > for me to discover that window->frame() was unsafe? e.g. neither
> > >
> >
>
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...
> > > nor
> > >
> >
>
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...
> > > has a comment saying anything's unsafe.
> > > 
> > > > Why do we need to check !executionContext->isDocument()?
> > > 
> > > The static_cast<Document*>(executionContext) is only valid if
> executionContext
> > > is a Document.
> > 
> > Sorry for not having a comment about it. We had to check
> > isCurrentlyDisplayedInFrame() before accessing window->frame() (c.f.,
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...),
> > but dcheng@ recently removed the restriction. (So actually it's now safe to
> > access window->frame().)
> > 
> > Sorry about the confusion. I'll clean up the code and add a good comment.
> 
> Sooo LGTM?

Would it be better to land the other CL
(https://codereview.chromium.org/1334763002/) first? Then we can use
fromExecutionContext() (i.e., we don't need to introduce fromScriptState()).

[ortuno, 2015-10-13 01:47:16]

On 2015/10/13 01:44:34, haraken wrote:
> On 2015/10/13 01:42:20, ortuno wrote:
> > On 2015/10/12 23:29:25, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/1399623003/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1399623003/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp:39: if
> > > (window && window->frame()) {
> > > On 2015/10/12 21:57:24, Jeffrey Yasskin wrote:
> > > > On 2015/10/12 17:33:52, ortuno wrote:
> > > > > On 2015/10/10 at 14:09:57, haraken wrote:
> > > > > > It is unsafe to use window->frame() directly. You need to use 
> > > > > toFrameIfNotDetached() in V8Binding.h.
> > > > 
> > > > Thanks Kentaro for catching my mistakes in writing this class. Was there
> any
> > > way
> > > > for me to discover that window->frame() was unsafe? e.g. neither
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...
> > > > nor
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...
> > > > has a comment saying anything's unsafe.
> > > > 
> > > > > Why do we need to check !executionContext->isDocument()?
> > > > 
> > > > The static_cast<Document*>(executionContext) is only valid if
> > executionContext
> > > > is a Document.
> > > 
> > > Sorry for not having a comment about it. We had to check
> > > isCurrentlyDisplayedInFrame() before accessing window->frame() (c.f.,
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...),
> > > but dcheng@ recently removed the restriction. (So actually it's now safe
to
> > > access window->frame().)
> > > 
> > > Sorry about the confusion. I'll clean up the code and add a good comment.
> > 
> > Sooo LGTM?
> 
> Would it be better to land the other CL
> (https://codereview.chromium.org/1334763002/) first? Then we can use
> fromExecutionContext() (i.e., we don't need to introduce fromScriptState()).

I still think we need fromScriptState(). Otherwise we would have to type
"fromExecutionContext(scriptState->executionContext())" a lot throughout our
code.

[haraken, 2015-10-13 02:05:09]

https://codereview.chromium.org/1399623003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp
(right):

https://codereview.chromium.org/1399623003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp:43: return
BluetoothSupplement::from(static_cast<Document*>(scriptState->executionContext())->frame());

After landing the other CL, you should replace the implementation with
fromExecutionContext(scriptState->executionContext()).

With that change, LGTM.

[ortuno, 2015-10-13 15:40:29]

https://codereview.chromium.org/1399623003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp
(right):

https://codereview.chromium.org/1399623003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp:43: return
BluetoothSupplement::from(static_cast<Document*>(scriptState->executionContext())->frame());
On 2015/10/13 02:05:09, haraken wrote:
> 
> After landing the other CL, you should replace the implementation with
> fromExecutionContext(scriptState->executionContext()).
> 
> With that change, LGTM.

This CL is meant to land first. The other CL changes the implementation to what
you suggested.

[haraken, 2015-10-13 15:41:43]

On 2015/10/13 15:40:29, ortuno wrote:
>
https://codereview.chromium.org/1399623003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp
> (right):
> 
>
https://codereview.chromium.org/1399623003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp:43: return
>
BluetoothSupplement::from(static_cast<Document*>(scriptState->executionContext())->frame());
> On 2015/10/13 02:05:09, haraken wrote:
> > 
> > After landing the other CL, you should replace the implementation with
> > fromExecutionContext(scriptState->executionContext()).
> > 
> > With that change, LGTM.
> 
> This CL is meant to land first. The other CL changes the implementation to
what
> you suggested.

ok

[ortuno, 2015-10-13 15:42:33]

ortuno@chromium.org changed reviewers:
+ japhet@chromium.org, jyasskin@chromium.org

[ortuno, 2015-10-13 15:42:34]

jyasskin: PTAL at Source/modules/bluetooth
japhet: PTAL public/platform/Platform.h

[Jeffrey Yasskin, 2015-10-13 15:52:07]

LGTM

I don't really see the need to rename BluetoothSupplement::from to
BluetoothSupplement::fromScriptState or to expose the unused from(LocalFrame*),
but I'm happy to follow Kentaro's suggestions.

[ortuno, 2015-10-14 15:19:32]

japhet: ping for public/platform/Platform.h

[Nate Chapin, 2015-10-14 16:48:15]

public/platform LGTM

[ortuno, 2015-10-14 16:48:54]

The CQ bit was checked by ortuno@chromium.org

[commit-bot: I haz the power, 2015-10-14 16:49:14]

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

[commit-bot: I haz the power, 2015-10-14 18:23:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-14 18:24:27]

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