Separate ContentSettingsClient out from LocalFrameClient

Separate ContentSettingsClient out from LocalFrameClient

LocalFrameClient is too overloaded, which is making it hard to figure out frame
dependency.  This CL tries to split ContentSettingsClient part out of LocalFrameClient
to clarify the dependency.  In this way it becomes possible to provide/implement 
ContentSettingsClient without Frame.

This change also moves WebContentSettingsClient from public/web to public/platform
for easier layering, but if anyone disagrees I can revert the part.

BUG=695279
TBR=jochen@chromium.org, boliu@chromium.org

Review-Url: https://codereview.chromium.org/2786673002
Cr-Commit-Position: refs/heads/master@{#462020}
Committed: https://chromium.googlesource.com/chromium/src/+/8bd5d6a15f00ab291c675c2e49b19275d263b6bc

[kinuko, 2017-03-29 15:46:44]

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

[kinuko, 2017-03-29 15:46:57]

Description was changed from

==========
platform

platform


hacking


ContentSettingsClient

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

to

==========
ContentSettingsClient

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

[commit-bot: I haz the power, 2017-03-29 15:47:16]

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-03-29 15:50:43]

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

[commit-bot: I haz the power, 2017-03-29 15:50:44]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[kinuko, 2017-03-29 15:58:59]

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

[commit-bot: I haz the power, 2017-03-29 15:59:38]

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-03-29 16:55:07]

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

[commit-bot: I haz the power, 2017-03-29 16:55:08]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[kinuko, 2017-03-31 10:01:54]

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

[commit-bot: I haz the power, 2017-03-31 10:02:02]

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-03-31 10:54:11]

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

[commit-bot: I haz the power, 2017-03-31 10:54:12]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[kinuko, 2017-03-31 12:55:48]

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

[commit-bot: I haz the power, 2017-03-31 12:56:04]

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

[kinuko, 2017-03-31 12:56:16]

Patchset #3 (id:40001) has been deleted

[kinuko, 2017-03-31 13:08:48]

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

[commit-bot: I haz the power, 2017-03-31 13:09:01]

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

[kinuko, 2017-03-31 13:14:31]

Patchset #3 (id:60001) has been deleted

[kinuko, 2017-03-31 13:32:58]

Description was changed from

==========
ContentSettingsClient

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

to

==========
Separate ContentSettingsClient out from LocalFrameClient

LocalFrameClient is too overloaded, which is making it hard to figure out frame
dependency.  This CL tries to split ContentSettingsClient part out of
LocalFrameClient
to clarify the dependency.  In this way ContentSettingsClient can be implemented
without Frame (e.g. we have WorkerContentSettingsClient but it should be
possible to
merge the interface after this patch).

This change also moves WebContentSettingsClient from public/web to
public/platform
for easier layering, but if anyone disagrees I can revert the change.

BUG=695279
==========

[kinuko, 2017-03-31 13:33:16]

Description was changed from

==========
Separate ContentSettingsClient out from LocalFrameClient

LocalFrameClient is too overloaded, which is making it hard to figure out frame
dependency.  This CL tries to split ContentSettingsClient part out of
LocalFrameClient
to clarify the dependency.  In this way ContentSettingsClient can be implemented
without Frame (e.g. we have WorkerContentSettingsClient but it should be
possible to
merge the interface after this patch).

This change also moves WebContentSettingsClient from public/web to
public/platform
for easier layering, but if anyone disagrees I can revert the change.

BUG=695279
==========

to

==========
Separate ContentSettingsClient out from LocalFrameClient

LocalFrameClient is too overloaded, which is making it hard to figure out frame
dependency.  This CL tries to split ContentSettingsClient part out of
LocalFrameClient
to clarify the dependency.  In this way ContentSettingsClient can be implemented
without Frame (e.g. we have WorkerContentSettingsClient but it should be
possible to
merge the interface after this patch).

This change also moves WebContentSettingsClient from public/web to
public/platform
for easier layering, but if anyone disagrees I can revert the part.

BUG=695279
==========

[kinuko, 2017-03-31 14:06:23]

Description was changed from

==========
Separate ContentSettingsClient out from LocalFrameClient

LocalFrameClient is too overloaded, which is making it hard to figure out frame
dependency.  This CL tries to split ContentSettingsClient part out of
LocalFrameClient
to clarify the dependency.  In this way ContentSettingsClient can be implemented
without Frame (e.g. we have WorkerContentSettingsClient but it should be
possible to
merge the interface after this patch).

This change also moves WebContentSettingsClient from public/web to
public/platform
for easier layering, but if anyone disagrees I can revert the part.

BUG=695279
==========

to

==========
Separate ContentSettingsClient out from LocalFrameClient

LocalFrameClient is too overloaded, which is making it hard to figure out frame
dependency.  This CL tries to split ContentSettingsClient part out of
LocalFrameClient
to clarify the dependency.  In this way it becomes possible to implement 
ContentSettingsClient without Frame.

This change also moves WebContentSettingsClient from public/web to
public/platform
for easier layering, but if anyone disagrees I can revert the part.

BUG=695279
==========

[commit-bot: I haz the power, 2017-03-31 14:29:49]

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

[commit-bot: I haz the power, 2017-03-31 14:29:50]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[kinuko, 2017-03-31 15:32:50]

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

[commit-bot: I haz the power, 2017-03-31 15:33:08]

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

[kinuko, 2017-03-31 16:05:01]

Description was changed from

==========
Separate ContentSettingsClient out from LocalFrameClient

LocalFrameClient is too overloaded, which is making it hard to figure out frame
dependency.  This CL tries to split ContentSettingsClient part out of
LocalFrameClient
to clarify the dependency.  In this way it becomes possible to implement 
ContentSettingsClient without Frame.

This change also moves WebContentSettingsClient from public/web to
public/platform
for easier layering, but if anyone disagrees I can revert the part.

BUG=695279
==========

to

==========
Separate ContentSettingsClient out from LocalFrameClient

LocalFrameClient is too overloaded, which is making it hard to figure out frame
dependency.  This CL tries to split ContentSettingsClient part out of
LocalFrameClient
to clarify the dependency.  In this way it becomes possible to provide/implement

ContentSettingsClient without Frame.

This change also moves WebContentSettingsClient from public/web to
public/platform
for easier layering, but if anyone disagrees I can revert the part.

BUG=695279
==========

[kinuko, 2017-03-31 16:06:56]

kinuko@chromium.org changed reviewers:
+ dcheng@chromium.org, slangley@chromium.org

[kinuko, 2017-03-31 16:13:12]

This has become a little bigger change than I had expected (let me know if this
conflicts with other plans).  What do you think?

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/BUILD.gn (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/BUILD.gn:49: "ContextFeaturesClientImpl.h",
Moved from Source/web as they no longer has dependency to web/

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webdatabase/DatabaseClient.cpp (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webdatabase/DatabaseClient.cpp:73: }
Moved the impl from DatabaseClientImpl and removed DatabaseClientImpl

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/publ...
File third_party/WebKit/public/BUILD.gn (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/publ...
third_party/WebKit/public/BUILD.gn:140: "platform/WebContentSettingsClient.h",
This one can be left in public/web if it's preferable, while having this under
platform makes layering easier.

[commit-bot: I haz the power, 2017-03-31 16:44:38]

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

[commit-bot: I haz the power, 2017-03-31 16:44:39]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[kinuko, 2017-04-01 02:34:49]

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

[commit-bot: I haz the power, 2017-04-01 02:34:59]

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-01 03:32:57]

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

[commit-bot: I haz the power, 2017-04-01 03:32:59]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-04-02 05:26:19]

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/ContentSettingsClient.h (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ContentSettingsClient.h:24: : public
GarbageCollected<ContentSettingsClient> {
FWIW, I'm not sure what the value of making this garbage collected is: it seems
like it could just be used directly

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrameClient.h (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrameClient.h:273: virtual
ContentSettingsClient* contentSettingsClient() = 0;
Can we make this return a reference?

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3991:
ContentSettingsClient* settingsClient = frame->contentSettingsClient();
Since we already check that we have a frame here, I think we shouldn't need to
do the null check for |settingsClient|.

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/EmptyClients.cpp (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/EmptyClients.cpp:200: return new
ContentSettingsClient();
It seems a little surprising that this creates a new one every time. Would it be
possible to just make this a member?

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:680: if (!client() ||
!m_frame->contentSettingsClient())
Similarly, I think we ideally shouldn't need to null check here (since we
checked that we have a client)

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webdatabase/DatabaseClient.cpp (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webdatabase/DatabaseClient.cpp:68: if
(document->frame()->contentSettingsClient()) {
Do we need to null-check that frame is not null?

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/IndexedDBClientImpl.cpp (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/web/IndexedDBClientImpl.cpp:68: if
(frame->contentSettingsClient()) {
If we have a frame, I think we can assume it is not detached.

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/LocalFileSystemClient.cpp (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/web/LocalFileSystemClient.cpp:76: if
(!document->frame()->contentSettingsClient()) {
Ditto: if we are assuming frame is not null here (I'm not sure why we can assume
that though?), then we shouldn't need to check that we have a settings client

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/StorageClientImpl.cpp (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/web/StorageClientImpl.cpp:59: return
!frame->contentSettingsClient() ||
The caller seems to check that frame is attached, so maybe we can DCHECK that we
have a content settings client here too.

[kinuko, 2017-04-03 07:42:35]

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

[commit-bot: I haz the power, 2017-04-03 07:42:49]

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-03 07:55:24]

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

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

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[kinuko, 2017-04-03 13:57:16]

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

[commit-bot: I haz the power, 2017-04-03 13:57:31]

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

[kinuko, 2017-04-03 14:01:54]

Patchset #5 (id:110001) has been deleted

[kinuko, 2017-04-03 14:04:34]

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

[kinuko, 2017-04-03 14:04:40]

Patchset #5 (id:130001) has been deleted

[commit-bot: I haz the power, 2017-04-03 14:04:51]

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

[kinuko, 2017-04-03 15:15:06]

Updated. PTAL

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/ContentSettingsClient.h (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ContentSettingsClient.h:24: : public
GarbageCollected<ContentSettingsClient> {
On 2017/04/02 05:26:19, dcheng wrote:
> FWIW, I'm not sure what the value of making this garbage collected is: it
seems
> like it could just be used directly

True. Hmm.  Made it not garbage collected in the next patch.

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrameClient.h (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrameClient.h:273: virtual
ContentSettingsClient* contentSettingsClient() = 0;
On 2017/04/02 05:26:19, dcheng wrote:
> Can we make this return a reference?

Done.

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3991:
ContentSettingsClient* settingsClient = frame->contentSettingsClient();
On 2017/04/02 05:26:19, dcheng wrote:
> Since we already check that we have a frame here, I think we shouldn't need to
> do the null check for |settingsClient|.

Done.

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/EmptyClients.cpp (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/EmptyClients.cpp:200: return new
ContentSettingsClient();
On 2017/04/02 05:26:19, dcheng wrote:
> It seems a little surprising that this creates a new one every time. Would it
be
> possible to just make this a member?

Oh yes, this was a tentative code I was going to fix. Done.

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:680: if (!client() ||
!m_frame->contentSettingsClient())
On 2017/04/02 05:26:19, dcheng wrote:
> Similarly, I think we ideally shouldn't need to null check here (since we
> checked that we have a client)

Done.

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webdatabase/DatabaseClient.cpp (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webdatabase/DatabaseClient.cpp:68: if
(document->frame()->contentSettingsClient()) {
On 2017/04/02 05:26:19, dcheng wrote:
> Do we need to null-check that frame is not null?

I think we do, done.

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/IndexedDBClientImpl.cpp (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/web/IndexedDBClientImpl.cpp:68: if
(frame->contentSettingsClient()) {
On 2017/04/02 05:26:19, dcheng wrote:
> If we have a frame, I think we can assume it is not detached.

Done.

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/LocalFileSystemClient.cpp (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/web/LocalFileSystemClient.cpp:76: if
(!document->frame()->contentSettingsClient()) {
On 2017/04/02 05:26:19, dcheng wrote:
> Ditto: if we are assuming frame is not null here (I'm not sure why we can
assume
> that though?), then we shouldn't need to check that we have a settings client

Added frame nullcheck, and removed contentSettingsClient nullcheck.

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/StorageClientImpl.cpp (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/web/StorageClientImpl.cpp:59: return
!frame->contentSettingsClient() ||
On 2017/04/02 05:26:19, dcheng wrote:
> The caller seems to check that frame is attached, so maybe we can DCHECK that
we
> have a content settings client here too.

Done.

[commit-bot: I haz the power, 2017-04-03 17:22:24]

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

[commit-bot: I haz the power, 2017-04-03 17:22:25]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-04-03 19:28:23]

LGTM with two nits

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webdatabase/DatabaseClient.cpp (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webdatabase/DatabaseClient.cpp:68: if
(document->frame()->contentSettingsClient()) {
On 2017/04/03 15:15:06, kinuko wrote:
> On 2017/04/02 05:26:19, dcheng wrote:
> > Do we need to null-check that frame is not null?
> 
> I think we do, done.

Btw, I didn't trace this back far enough: the only callstack that calls into
this seems to be:
  DatabaseClient::allowDatabase()
  DatabaseTracker::canEstablishConnection()
  DatabaseManager::openDatabaseInternal()
  DatabaseManager::openDatabase()
  DOMWindowWebDatabase::openDatabase()

and DOMWindowWebDatabase::openDatabase() checks
DOMWindow::isCurrentlyDisplayedInFrame(), which checks that frame is not null.

So we actually don't need this...

(Maybe we can do a DCHECK? Up to you)

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/LocalFileSystemClient.cpp (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/web/LocalFileSystemClient.cpp:76: if
(!document->frame()->contentSettingsClient()) {
On 2017/04/03 15:15:06, kinuko wrote:
> On 2017/04/02 05:26:19, dcheng wrote:
> > Ditto: if we are assuming frame is not null here (I'm not sure why we can
> assume
> > that though?), then we shouldn't need to check that we have a settings
client
> 
> Added frame nullcheck, and removed contentSettingsClient nullcheck.

Similarly, I did some more tracing back and the only Document caller is from
DOMWindowFileSystem::webkitRequestFileSystem, which also checks
DOMWindow::isCurrentlyDisplayedInFrame(). So we can also DCHECK this.

[kinuko, 2017-04-04 03:26:23]

Thanks! Done.

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webdatabase/DatabaseClient.cpp (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webdatabase/DatabaseClient.cpp:68: if
(document->frame()->contentSettingsClient()) {
On 2017/04/03 19:28:23, dcheng wrote:
> On 2017/04/03 15:15:06, kinuko wrote:
> > On 2017/04/02 05:26:19, dcheng wrote:
> > > Do we need to null-check that frame is not null?
> > 
> > I think we do, done.
> 
> Btw, I didn't trace this back far enough: the only callstack that calls into
> this seems to be:
>   DatabaseClient::allowDatabase()
>   DatabaseTracker::canEstablishConnection()
>   DatabaseManager::openDatabaseInternal()
>   DatabaseManager::openDatabase()
>   DOMWindowWebDatabase::openDatabase()
> 
> and DOMWindowWebDatabase::openDatabase() checks
> DOMWindow::isCurrentlyDisplayedInFrame(), which checks that frame is not null.
> 
> So we actually don't need this...
> 
> (Maybe we can do a DCHECK? Up to you)

Thanks for looking!  I turned this into DCHECK.

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/LocalFileSystemClient.cpp (right):

https://codereview.chromium.org/2786673002/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/web/LocalFileSystemClient.cpp:76: if
(!document->frame()->contentSettingsClient()) {
On 2017/04/03 19:28:23, dcheng wrote:
> On 2017/04/03 15:15:06, kinuko wrote:
> > On 2017/04/02 05:26:19, dcheng wrote:
> > > Ditto: if we are assuming frame is not null here (I'm not sure why we can
> > assume
> > > that though?), then we shouldn't need to check that we have a settings
> client
> > 
> > Added frame nullcheck, and removed contentSettingsClient nullcheck.
> 
> Similarly, I did some more tracing back and the only Document caller is from
> DOMWindowFileSystem::webkitRequestFileSystem, which also checks
> DOMWindow::isCurrentlyDisplayedInFrame(). So we can also DCHECK this.

Done.

[kinuko, 2017-04-04 03:29:45]

Description was changed from

==========
Separate ContentSettingsClient out from LocalFrameClient

LocalFrameClient is too overloaded, which is making it hard to figure out frame
dependency.  This CL tries to split ContentSettingsClient part out of
LocalFrameClient
to clarify the dependency.  In this way it becomes possible to provide/implement

ContentSettingsClient without Frame.

This change also moves WebContentSettingsClient from public/web to
public/platform
for easier layering, but if anyone disagrees I can revert the part.

BUG=695279
==========

to

==========
Separate ContentSettingsClient out from LocalFrameClient

LocalFrameClient is too overloaded, which is making it hard to figure out frame
dependency.  This CL tries to split ContentSettingsClient part out of
LocalFrameClient
to clarify the dependency.  In this way it becomes possible to provide/implement

ContentSettingsClient without Frame.

This change also moves WebContentSettingsClient from public/web to
public/platform
for easier layering, but if anyone disagrees I can revert the part.

BUG=695279
TBR=jochen@chromium.org, boliu@chromium.org
==========

[kinuko, 2017-04-04 03:29:45]

kinuko@chromium.org changed reviewers:
+ boliu@chromium.org, haraken@chromium.org, jochen@chromium.org

[kinuko, 2017-04-04 03:30:15]

kinuko@chromium.org changed reviewers:
+ michaeln@chromium.org

[kinuko, 2017-04-04 03:36:18]

michaeln@ and/or haraken@: could you review changes in webdatabase

Would do TBR for following changes (header-location change only):
boliu@: android_webview
jochen@: chrome/renderer

[haraken, 2017-04-04 11:25:32]

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ContentSettingsClient.cpp (right):

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ContentSettingsClient.cpp:64: return
enabledPerSettings;

Are we intending to return what's passed in if m_client is null? I guess it
would be more intuitive to return false if m_client is null.

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:339: return frame()
? frame()->contentSettingsClient() : nullptr;

Can frame() be null? localFrameClient() assumes that it's never null.

I'd prefer removing FrameFetchContext::contentSettingsClient().

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/LocalFrameClientImpl.cpp (right):

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/LocalFrameClientImpl.cpp:894:
ContentSettingsClient& LocalFrameClientImpl::contentSettingsClient() {

Nit: I'd prefer removing this helper.

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/StorageClientImpl.cpp (right):

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/StorageClientImpl.cpp:44:
ContentSettingsClient::StorageType::kSession);

Can we make this change in a separate CL? I think it's nicer to add the macro to
wtf/.

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/StorageClientImpl.cpp:61:
static_cast<ContentSettingsClient::StorageType>(type));

Shouldn't this be type == LocalStorage ?

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.h (right):

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.h:476: ContentSettingsClient
m_contentSettingsClient;

I'm wondering if WebLocalFrameImpl is a right object to own
ContentSettingsClient. Would you help me understand why you chose
WebLocalFrameImpl?

In the first place, do we really need ContentSettingsClient? Now that we have
WebContentSettingsClient on platform/, can we probably directly use
WebContentSettingsClient to configure content settings?

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebContentSettingsClient.h (right):

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebContentSettingsClient.h:16: class
WebContentSettingsClient {

Add a class-level comment.

[kinuko, 2017-04-04 14:50:57]

Thanks.

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ContentSettingsClient.cpp (right):

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ContentSettingsClient.cpp:64: return
enabledPerSettings;
On 2017/04/04 11:25:32, haraken wrote:
> 
> Are we intending to return what's passed in if m_client is null? I guess it
> would be more intuitive to return false if m_client is null. 

The given value is basically the default value and the callsite is supposed to
pass the value from Settings.  If we change it that could cause behavioral
changes.

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:339: return frame()
? frame()->contentSettingsClient() : nullptr;
On 2017/04/04 11:25:32, haraken wrote:
> 
> Can frame() be null? localFrameClient() assumes that it's never null.
> 
> I'd prefer removing FrameFetchContext::contentSettingsClient().

I removed the null check.  I'd like to keep this method as I plan to refactor
this class further along the line.

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/LocalFrameClientImpl.cpp (right):

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/LocalFrameClientImpl.cpp:894:
ContentSettingsClient& LocalFrameClientImpl::contentSettingsClient() {
On 2017/04/04 11:25:32, haraken wrote:
> 
> Nit: I'd prefer removing this helper.

Actually I wanted to do so too, but having this helper made things a lot easier
for testing plumbing.  (I suppose frame cleanup project will be incrementally
cleaning up these methods too anyways)

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/StorageClientImpl.cpp (right):

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/StorageClientImpl.cpp:44:
ContentSettingsClient::StorageType::kSession);
On 2017/04/04 11:25:32, haraken wrote:
> 
> Can we make this change in a separate CL? I think it's nicer to add the macro
to
> wtf/.

I could try adding these macros in a separate CL.  Or do you mean you want to
keep ContentSettingsClient::canAccessStorage take boolean instead of enum?

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/StorageClientImpl.cpp:61:
static_cast<ContentSettingsClient::StorageType>(type));
On 2017/04/04 11:25:32, haraken wrote:
> 
> Shouldn't this be type == LocalStorage ?

ContentSettingsClient's allowStorage does that inside. (I wanted to make it take
enum instead bool)

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.h (right):

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.h:476: ContentSettingsClient
m_contentSettingsClient;
On 2017/04/04 11:25:32, haraken wrote:
> 
> I'm wondering if WebLocalFrameImpl is a right object to own
> ContentSettingsClient. Would you help me understand why you chose
> WebLocalFrameImpl?

Yep- I don't think this would be the best place either.  I tried several
alternatives but this looked least disruptive yet somewhat acceptable as an
incremental step.

One of the complications is that WebLocalFrameImpl's initialization order and
LocalFrame creation time is not consistent in different cases, so we anyway need
to keep something in this class to make things always work.  I assume all those
would need to be incrementally cleaned up.

> In the first place, do we really need ContentSettingsClient? Now that we have
> WebContentSettingsClient on platform/, can we probably directly use
> WebContentSettingsClient to configure content settings?

(Fwiw I had wondered the same.)  What ContentSettingsClient does (and what
LocalFrameClient was doing in the aspect) is to return the given default value
if embedder doesn't install its own client, so it basically hides embedder's
implementation details (btw here embedder means chrome etc, not content).   We
can try removing the wrapping part too, but it'd make this change even bigger &
the callsite's code a little more complex.  I want to avoid making the change at
least in this CL.

Separately from that I could make ContentSettingsClient a subclass of
WebContentSettingsClient and name it WrappedWebContentSettingsClient or
something like that to make it a little clearer what it does, if it helps.

[boliu, 2017-04-04 15:35:34]

android_webview rs lgtm

[michaeln, 2017-04-04 23:45:08]

https://codereview.chromium.org/2786673002/diff/190001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webdatabase/DatabaseClient.cpp (right):

https://codereview.chromium.org/2786673002/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webdatabase/DatabaseClient.cpp:68:
DCHECK(document->frame());
lgtm... but the old code returned false in case the frame was null, to be
safe(r) maybe early return false in that case here too

[kinuko, 2017-04-05 01:13:59]

On 2017/04/04 23:45:08, michaeln wrote:
>
https://codereview.chromium.org/2786673002/diff/190001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/webdatabase/DatabaseClient.cpp (right):
> 
>
https://codereview.chromium.org/2786673002/diff/190001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/webdatabase/DatabaseClient.cpp:68:
> DCHECK(document->frame());
> lgtm... but the old code returned false in case the frame was null, to be
> safe(r) maybe early return false in that case here too

Yeah... dcheng@ checked it shouldn't be the case.  I think we want to rather
only have this DCHECK and fix crashes (or change the code) if we have any.

[haraken, 2017-04-05 02:31:08]

LGTM as an incremental step to clean up ContentSettingsClient.

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.h (right):

https://codereview.chromium.org/2786673002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.h:476: ContentSettingsClient
m_contentSettingsClient;
On 2017/04/04 14:50:57, kinuko wrote:
> On 2017/04/04 11:25:32, haraken wrote:
> > 
> > I'm wondering if WebLocalFrameImpl is a right object to own
> > ContentSettingsClient. Would you help me understand why you chose
> > WebLocalFrameImpl?
> 
> Yep- I don't think this would be the best place either.  I tried several
> alternatives but this looked least disruptive yet somewhat acceptable as an
> incremental step.
> 
> One of the complications is that WebLocalFrameImpl's initialization order and
> LocalFrame creation time is not consistent in different cases, so we anyway
need
> to keep something in this class to make things always work.  I assume all
those
> would need to be incrementally cleaned up.
> 
> > In the first place, do we really need ContentSettingsClient? Now that we
have
> > WebContentSettingsClient on platform/, can we probably directly use
> > WebContentSettingsClient to configure content settings?
> 
> (Fwiw I had wondered the same.)  What ContentSettingsClient does (and what
> LocalFrameClient was doing in the aspect) is to return the given default value
> if embedder doesn't install its own client, so it basically hides embedder's
> implementation details (btw here embedder means chrome etc, not content).   We
> can try removing the wrapping part too, but it'd make this change even bigger
&
> the callsite's code a little more complex.  I want to avoid making the change
at
> least in this CL.
> 
> Separately from that I could make ContentSettingsClient a subclass of
> WebContentSettingsClient and name it WrappedWebContentSettingsClient or
> something like that to make it a little clearer what it does, if it helps.

Yeah, I want to think about cleaning up these things in follow-up CLs.

[kinuko, 2017-04-05 07:09:34]

Thanks!

[kinuko, 2017-04-05 07:09:42]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2017-04-05 07:09:43]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
michaeln@chromium.org, boliu@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2786673002/#ps210001
(title: "added class-level comment")

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

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

[jochen (gone - plz use gerrit), 2017-04-05 08:34:29]

chrome/renderer lgtm

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

CQ is committing da patch.

Bot data: {"patchset_id": 210001, "attempt_start_ts": 1491376182438930,
"parent_rev": "19a2c47d9717b8f83d8ad79e914bd972eb1c1311", "commit_rev":
"8bd5d6a15f00ab291c675c2e49b19275d263b6bc"}

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

Description was changed from

==========
Separate ContentSettingsClient out from LocalFrameClient

LocalFrameClient is too overloaded, which is making it hard to figure out frame
dependency.  This CL tries to split ContentSettingsClient part out of
LocalFrameClient
to clarify the dependency.  In this way it becomes possible to provide/implement

ContentSettingsClient without Frame.

This change also moves WebContentSettingsClient from public/web to
public/platform
for easier layering, but if anyone disagrees I can revert the part.

BUG=695279
TBR=jochen@chromium.org, boliu@chromium.org
==========

to

==========
Separate ContentSettingsClient out from LocalFrameClient

LocalFrameClient is too overloaded, which is making it hard to figure out frame
dependency.  This CL tries to split ContentSettingsClient part out of
LocalFrameClient
to clarify the dependency.  In this way it becomes possible to provide/implement

ContentSettingsClient without Frame.

This change also moves WebContentSettingsClient from public/web to
public/platform
for easier layering, but if anyone disagrees I can revert the part.

BUG=695279
TBR=jochen@chromium.org, boliu@chromium.org

Review-Url: https://codereview.chromium.org/2786673002
Cr-Commit-Position: refs/heads/master@{#462020}
Committed:
https://chromium.googlesource.com/chromium/src/+/8bd5d6a15f00ab291c675c2e49b1...
==========

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

Committed patchset #8 (id:210001) as
https://chromium.googlesource.com/chromium/src/+/8bd5d6a15f00ab291c675c2e49b1...