Move file chooser from RenderView(Host) to RenderFrame(Host).

Move file chooser from RenderView(Host) to RenderFrame(Host).

The file chooser does not work with out-of-process iframes, since it works through RenderView(Host). It needs to be moved to user RenderFrame(Host) at which point it works for subframes in a process different from its parent.

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

TBR=asvitkine@chromium.org

Committed: https://crrev.com/ada75b29b3ecd8cb61c357befd50f24f2e50d319
Cr-Commit-Position: refs/heads/master@{#399373}

[nasko, 2016-06-09 17:15:35]

Description was changed from

==========
Move file chooser from RenderView(Host) to RenderFrame(Host).

BUG=615575
==========

to

==========
Move file chooser from RenderView(Host) to RenderFrame(Host).

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

[nasko, 2016-06-10 20:48:49]

Patchset #3 (id:40001) has been deleted

[nasko, 2016-06-10 21:42:39]

Description was changed from

==========
Move file chooser from RenderView(Host) to RenderFrame(Host).

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

to

==========
Move file chooser from RenderView(Host) to RenderFrame(Host).

The file chooser does not work with out-of-process iframes, since it works
through RenderView(Host). It needs to be moved to user RenderFrame(Host) at
which point it works for subframes in a process different from its parent.

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

[dcheng, 2016-06-10 21:43:35]

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

[dcheng, 2016-06-10 21:43:36]

LGTM with nits

https://codereview.chromium.org/2050623005/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/2050623005/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameClient.h:448: WebFileChooserCompletion*
chooserCompletion) { return false; }
Technically I think you need to include or forward declare this too.

https://codereview.chromium.org/2050623005/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebViewClient.h (right):

https://codereview.chromium.org/2050623005/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebViewClient.h:40: #include
"WebFileChooserParams.h"
Can we remove these includes?

[dcheng, 2016-06-10 21:44:07]

To clarify, my LGTM is for Blink: i didn't examine any other files.

[nasko, 2016-06-10 21:46:07]

nasko@chromium.org changed reviewers:
+ avi@chromium.org, dmazzoni@chromium.org

[nasko, 2016-06-10 21:46:08]

Hey Avi and Dominic, 
Can you review this CL for me?

dmazzoni@ -> components/test_runner/
avi@ -> content/

Thanks in advance!
Nasko

[dmazzoni, 2016-06-10 21:49:06]

lgtm for test_runner

[Avi (use Gerrit), 2016-06-10 21:52:55]

LGTM with nits

https://codereview.chromium.org/2050623005/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2050623005/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1536:
bad_message::RVH_FILE_CHOOSER_PATH);
Have we renamed bad messages that we move from RVH to RFH?

https://codereview.chromium.org/2050623005/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2650: for (size_t i = 0; i
< files.size(); ++i) {
for (const auto& file : files)

https://codereview.chromium.org/2050623005/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2050623005/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:2253: // TODO(brettw) we might possibly
want to require a user gesture to open
TODO(brettw): We ...

https://codereview.chromium.org/2050623005/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:3813: for (size_t i = 0; i <
params.acceptTypes.size(); ++i)
for ( : )

https://codereview.chromium.org/2050623005/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:5219: for (size_t i = 0; i < files.size();
++i) {
for ( : )

[nasko, 2016-06-10 22:07:48]

nasko@chromium.org changed reviewers:
+ asvitkine@chromium.org

[nasko, 2016-06-10 22:07:50]

Fixed all nits.

Will TBR asvitkine@, since the histogram change is a single character
replacement.

https://codereview.chromium.org/2050623005/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2050623005/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1536:
bad_message::RVH_FILE_CHOOSER_PATH);
On 2016/06/10 21:52:54, Avi (OOO 12 June - 25 June) wrote:
> Have we renamed bad messages that we move from RVH to RFH?

I don't know, but it is a good thing to fix. Done.

https://codereview.chromium.org/2050623005/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2650: for (size_t i = 0; i
< files.size(); ++i) {
On 2016/06/10 21:52:54, Avi (OOO 12 June - 25 June) wrote:
> for (const auto& file : files)

Done.

https://codereview.chromium.org/2050623005/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2050623005/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:2253: // TODO(brettw) we might possibly
want to require a user gesture to open
On 2016/06/10 21:52:54, Avi (OOO 12 June - 25 June) wrote:
> TODO(brettw): We ...

Done.

https://codereview.chromium.org/2050623005/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:3813: for (size_t i = 0; i <
params.acceptTypes.size(); ++i)
On 2016/06/10 21:52:54, Avi (OOO 12 June - 25 June) wrote:
> for ( : )

Done.

https://codereview.chromium.org/2050623005/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:5219: for (size_t i = 0; i < files.size();
++i) {
On 2016/06/10 21:52:54, Avi (OOO 12 June - 25 June) wrote:
> for ( : )

This one uses i for the WebVector access, so I kept it as is.

https://codereview.chromium.org/2050623005/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/2050623005/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameClient.h:448: WebFileChooserCompletion*
chooserCompletion) { return false; }
On 2016/06/10 21:43:35, dcheng wrote:
> Technically I think you need to include or forward declare this too.

Done.

https://codereview.chromium.org/2050623005/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebViewClient.h (right):

https://codereview.chromium.org/2050623005/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebViewClient.h:40: #include
"WebFileChooserParams.h"
On 2016/06/10 21:43:35, dcheng wrote:
> Can we remove these includes?

Done.

[nasko, 2016-06-10 22:08:23]

Description was changed from

==========
Move file chooser from RenderView(Host) to RenderFrame(Host).

The file chooser does not work with out-of-process iframes, since it works
through RenderView(Host). It needs to be moved to user RenderFrame(Host) at
which point it works for subframes in a process different from its parent.

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

to

==========
Move file chooser from RenderView(Host) to RenderFrame(Host).

The file chooser does not work with out-of-process iframes, since it works
through RenderView(Host). It needs to be moved to user RenderFrame(Host) at
which point it works for subframes in a process different from its parent.

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

TBR=asvitkine@chromium.org
==========

[nasko, 2016-06-10 22:08:34]

The CQ bit was checked by nasko@chromium.org

[nasko, 2016-06-10 22:08:34]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
dmazzoni@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2050623005/#ps100001
(title: "Fixes based on reviews.")

[commit-bot: I haz the power, 2016-06-10 22:09:55]

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

[Alexei Svitkine (slow), 2016-06-10 22:11:45]

histograms lgtm assuming you're not changing the meaning but just renaming the
same semantic behavior.

If you're changing the meaning, it's better to create another enum so that data
from different versions arent aggregated together.

[nasko, 2016-06-10 22:14:44]

On 2016/06/10 22:11:45, Alexei Svitkine (OOO) wrote:
> histograms lgtm assuming you're not changing the meaning but just renaming the
> same semantic behavior.
> 
> If you're changing the meaning, it's better to create another enum so that
data
> from different versions arent aggregated together.

I'm not changing the meaning, but the code is moving from one object (RVH) to
another (RFH), so it is best for the histogram constant to match the class name.
Thanks!

[Alexei Svitkine (slow), 2016-06-10 22:18:10]

sg

On Fri, Jun 10, 2016 at 6:14 PM, <nasko@chromium.org> wrote:

> On 2016/06/10 22:11:45, Alexei Svitkine (OOO) wrote:
> > histograms lgtm assuming you're not changing the meaning but just
> renaming the
> > same semantic behavior.
> >
> > If you're changing the meaning, it's better to create another enum so
> that
> data
> > from different versions arent aggregated together.
>
> I'm not changing the meaning, but the code is moving from one object (RVH)
> to
> another (RFH), so it is best for the histogram constant to match the class
> name.
> Thanks!
>
> https://codereview.chromium.org/2050623005/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Alexei Svitkine (slow), 2016-06-10 22:18:10]

sg

On Fri, Jun 10, 2016 at 6:14 PM, <nasko@chromium.org> wrote:

> On 2016/06/10 22:11:45, Alexei Svitkine (OOO) wrote:
> > histograms lgtm assuming you're not changing the meaning but just
> renaming the
> > same semantic behavior.
> >
> > If you're changing the meaning, it's better to create another enum so
> that
> data
> > from different versions arent aggregated together.
>
> I'm not changing the meaning, but the code is moving from one object (RVH)
> to
> another (RFH), so it is best for the histogram constant to match the class
> name.
> Thanks!
>
> https://codereview.chromium.org/2050623005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

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

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

[commit-bot: I haz the power, 2016-06-10 23:53:52]

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

[nasko, 2016-06-11 00:33:46]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2016-06-11 00:34:27]

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

[commit-bot: I haz the power, 2016-06-11 02:20:29]

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

[commit-bot: I haz the power, 2016-06-11 02:20:31]

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_...)

[nasko, 2016-06-11 08:51:31]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2016-06-11 08:51:47]

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

[commit-bot: I haz the power, 2016-06-11 16:09:59]

Description was changed from

==========
Move file chooser from RenderView(Host) to RenderFrame(Host).

The file chooser does not work with out-of-process iframes, since it works
through RenderView(Host). It needs to be moved to user RenderFrame(Host) at
which point it works for subframes in a process different from its parent.

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

TBR=asvitkine@chromium.org
==========

to

==========
Move file chooser from RenderView(Host) to RenderFrame(Host).

The file chooser does not work with out-of-process iframes, since it works
through RenderView(Host). It needs to be moved to user RenderFrame(Host) at
which point it works for subframes in a process different from its parent.

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

TBR=asvitkine@chromium.org
==========

[commit-bot: I haz the power, 2016-06-11 16:10:01]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-06-11 16:10:04]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-11 16:12:02]

Description was changed from

==========
Move file chooser from RenderView(Host) to RenderFrame(Host).

The file chooser does not work with out-of-process iframes, since it works
through RenderView(Host). It needs to be moved to user RenderFrame(Host) at
which point it works for subframes in a process different from its parent.

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

TBR=asvitkine@chromium.org
==========

to

==========
Move file chooser from RenderView(Host) to RenderFrame(Host).

The file chooser does not work with out-of-process iframes, since it works
through RenderView(Host). It needs to be moved to user RenderFrame(Host) at
which point it works for subframes in a process different from its parent.

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

TBR=asvitkine@chromium.org

Committed: https://crrev.com/ada75b29b3ecd8cb61c357befd50f24f2e50d319
Cr-Commit-Position: refs/heads/master@{#399373}
==========

[commit-bot: I haz the power, 2016-06-11 16:12:04]

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