Moving extension code out of "components/" to avoid layering violations.

Moving extension code out of "components/" to avoid layering violations.

component should not know and access extensions code directly.

Test=None, internal cleanup.

BUG=477096
Committed: https://crrev.com/b49c1e93eaf4182b629ce36b2feaea2f60b1e5f4
Cr-Commit-Position: refs/heads/master@{#328342}

[Deepak, 2015-04-30 12:20:32]

deepak.m1@samsung.com changed reviewers:
+ lazyboy@chromium.org

[Deepak, 2015-04-30 12:20:33]

Please review.

[lazyboy, 2015-04-30 16:33:31]

Thanks for the CL.

Could you also update the CL description adding a Test= section? Sth like:
Test=None, internal cleanup.

https://chromiumcodereview.appspot.com/1117893002/diff/1/chrome/browser/guest...
File chrome/browser/guest_view/web_view/context_menu_content_type_web_view.h
(right):

https://chromiumcodereview.appspot.com/1117893002/diff/1/chrome/browser/guest...
chrome/browser/guest_view/web_view/context_menu_content_type_web_view.h:28:
const extensions::Extension* GetExtension() const;
This should be private. Also the other GetExtension() function.

https://chromiumcodereview.appspot.com/1117893002/diff/1/chrome/browser/rende...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://chromiumcodereview.appspot.com/1117893002/diff/1/chrome/browser/rende...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:106: #if
defined(ENABLE_PRINTING)
Move the added include before this one (sorted).

https://chromiumcodereview.appspot.com/1117893002/diff/1/chrome/browser/rende...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:357:
embedder_web_contents_ = GetWebContentsToUse(source_web_contents_);
Can we move this to member initializer list above?

https://chromiumcodereview.appspot.com/1117893002/diff/1/chrome/browser/rende...
File chrome/browser/renderer_context_menu/render_view_context_menu.h (right):

https://chromiumcodereview.appspot.com/1117893002/diff/1/chrome/browser/rende...
chrome/browser/renderer_context_menu/render_view_context_menu.h:173: //
source_web_contents_.
|source_web_contents_|

https://chromiumcodereview.appspot.com/1117893002/diff/1/components/renderer_...
File components/renderer_context_menu/context_menu_content_type.cc (left):

https://chromiumcodereview.appspot.com/1117893002/diff/1/components/renderer_...
components/renderer_context_menu/context_menu_content_type.cc:12: #if
defined(ENABLE_EXTENSIONS)
Since we're moving extensions/ dependency in this components, you'd
also want to update components/renderer_context_menu/DEPS and
remove extensions/* whitelisting.

https://chromiumcodereview.appspot.com/1117893002/diff/1/components/renderer_...
File components/renderer_context_menu/context_menu_content_type.h (right):

https://chromiumcodereview.appspot.com/1117893002/diff/1/components/renderer_...
components/renderer_context_menu/context_menu_content_type.h:16: namespace
extensions {
Remove.

https://chromiumcodereview.appspot.com/1117893002/diff/1/components/renderer_...
components/renderer_context_menu/context_menu_content_type.h:71:
content::WebContents* source_web_contents() const {
Ah this is sad, can you add a TODO(lazyboy) to make this return const
WebContents*.

[Deepak, 2015-05-02 06:25:21]

Thanks for review.
Changes done as suggested.
PTAL.

https://codereview.chromium.org/1117893002/diff/1/chrome/browser/guest_view/w...
File chrome/browser/guest_view/web_view/context_menu_content_type_web_view.h
(right):

https://codereview.chromium.org/1117893002/diff/1/chrome/browser/guest_view/w...
chrome/browser/guest_view/web_view/context_menu_content_type_web_view.h:28:
const extensions::Extension* GetExtension() const;
On 2015/04/30 16:33:31, lazyboy wrote:
> This should be private. Also the other GetExtension() function.

Done.

https://codereview.chromium.org/1117893002/diff/1/chrome/browser/renderer_con...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1117893002/diff/1/chrome/browser/renderer_con...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:106: #if
defined(ENABLE_PRINTING)
On 2015/04/30 16:33:31, lazyboy wrote:
> Move the added include before this one (sorted).

Done.

https://codereview.chromium.org/1117893002/diff/1/chrome/browser/renderer_con...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:357:
embedder_web_contents_ = GetWebContentsToUse(source_web_contents_);
On 2015/04/30 16:33:31, lazyboy wrote:
> Can we move this to member initializer list above?

Done.

https://codereview.chromium.org/1117893002/diff/1/chrome/browser/renderer_con...
File chrome/browser/renderer_context_menu/render_view_context_menu.h (right):

https://codereview.chromium.org/1117893002/diff/1/chrome/browser/renderer_con...
chrome/browser/renderer_context_menu/render_view_context_menu.h:173: //
source_web_contents_.
On 2015/04/30 16:33:31, lazyboy wrote:
> |source_web_contents_|

Done.

https://codereview.chromium.org/1117893002/diff/1/components/renderer_context...
File components/renderer_context_menu/context_menu_content_type.cc (left):

https://codereview.chromium.org/1117893002/diff/1/components/renderer_context...
components/renderer_context_menu/context_menu_content_type.cc:12: #if
defined(ENABLE_EXTENSIONS)
On 2015/04/30 16:33:31, lazyboy wrote:
> Since we're moving extensions/ dependency in this components, you'd
> also want to update components/renderer_context_menu/DEPS and
> remove extensions/* whitelisting.

Done.

https://codereview.chromium.org/1117893002/diff/1/components/renderer_context...
File components/renderer_context_menu/context_menu_content_type.h (right):

https://codereview.chromium.org/1117893002/diff/1/components/renderer_context...
components/renderer_context_menu/context_menu_content_type.h:16: namespace
extensions {
On 2015/04/30 16:33:31, lazyboy wrote:
> Remove.

Done.

https://codereview.chromium.org/1117893002/diff/1/components/renderer_context...
components/renderer_context_menu/context_menu_content_type.h:71:
content::WebContents* source_web_contents() const {
On 2015/04/30 16:33:31, lazyboy wrote:
> Ah this is sad, can you add a TODO(lazyboy) to make this return const
> WebContents*.

Done.

[lazyboy, 2015-05-02 07:17:15]

LGTM with a nit.

https://codereview.chromium.org/1117893002/diff/20001/components/renderer_con...
File components/renderer_context_menu/context_menu_content_type.h (right):

https://codereview.chromium.org/1117893002/diff/20001/components/renderer_con...
components/renderer_context_menu/context_menu_content_type.h:67: //
TODO(lazyboy): Return const content::Webcontents pointer.
nit: change "Webcontents pointer" to "WebContents*"

[Deepak, 2015-05-02 07:56:24]

nit addressed.
Thanks.

https://codereview.chromium.org/1117893002/diff/20001/components/renderer_con...
File components/renderer_context_menu/context_menu_content_type.h (right):

https://codereview.chromium.org/1117893002/diff/20001/components/renderer_con...
components/renderer_context_menu/context_menu_content_type.h:67: //
TODO(lazyboy): Return const content::Webcontents pointer.
On 2015/05/02 07:17:15, lazyboy wrote:
> nit: change "Webcontents pointer" to "WebContents*"

Done.

[Deepak, 2015-05-02 07:56:43]

The CQ bit was checked by deepak.m1@samsung.com

[Deepak, 2015-05-02 07:56:43]

The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org
Link to the patchset: https://codereview.chromium.org/1117893002/#ps40001
(title: "Addressing nit.")

[commit-bot: I haz the power, 2015-05-02 07:56:59]

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

[Deepak, 2015-05-02 08:00:25]

The CQ bit was unchecked by deepak.m1@samsung.com

[Deepak, 2015-05-02 08:09:57]

deepak.m1@samsung.com changed reviewers:
+ fsamuel@chromium.org, jochen@chromium.org

[Deepak, 2015-05-02 08:09:58]

Dear @fsamuel and @jochen

Please give approval for below files.

++ fsamuel for 
guest_view/web_view/context_menu_content_type_web_view.h 
guest_view/web_view/context_menu_content_type_web_view.cpp
 
++ jochen for 
components/renderer_context_menu/*

Thanks

[Fady Samuel, 2015-05-03 03:08:46]

https://codereview.chromium.org/1117893002/diff/40001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/context_menu_content_type_web_view.cc
(right):

https://codereview.chromium.org/1117893002/diff/40001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/context_menu_content_type_web_view.cc:15: #if
defined(ENABLE_EXTENSIONS)
This (#if defined) is unnecessary. <webview>s are a part of the extensions
module.

https://codereview.chromium.org/1117893002/diff/40001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/context_menu_content_type_web_view.cc:29: #if
defined(ENABLE_EXTENSIONS)
This (#if defined) is unnecessary. <webview>s are a part of the extensions
module.

[Deepak, 2015-05-04 04:10:10]

@fsamuel
Thanks for review.
Changes done as suggested.
PTAL

https://codereview.chromium.org/1117893002/diff/40001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/context_menu_content_type_web_view.cc
(right):

https://codereview.chromium.org/1117893002/diff/40001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/context_menu_content_type_web_view.cc:15: #if
defined(ENABLE_EXTENSIONS)
On 2015/05/03 03:08:45, Fady Samuel wrote:
> This (#if defined) is unnecessary. <webview>s are a part of the extensions
> module. 

Done.

https://codereview.chromium.org/1117893002/diff/40001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/context_menu_content_type_web_view.cc:29: #if
defined(ENABLE_EXTENSIONS)
On 2015/05/03 03:08:45, Fady Samuel wrote:
> This (#if defined) is unnecessary. <webview>s are a part of the extensions
> module. 

Done.

[Fady Samuel, 2015-05-04 04:16:10]

https://codereview.chromium.org/1117893002/diff/60001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1117893002/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:314:
extensions::MimeHandlerViewGuest::FromWebContents(web_contents);
Taking a look at this code now: is this actually what you want?

What if you have a PDF inside a <webview>? Would using
GuestViewBase::GetTopLevelWebContents be more appropriate? Istiaque (lazyboy@)
might have some thoughts on this as well. I'm missing context on what the
original problem was that this fixed.

[Deepak, 2015-05-04 06:06:45]

PTAL

https://codereview.chromium.org/1117893002/diff/60001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1117893002/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:314:
extensions::MimeHandlerViewGuest::FromWebContents(web_contents);
On 2015/05/04 04:16:10, Fady Samuel wrote:
> Taking a look at this code now: is this actually what you want?
> 
> What if you have a PDF inside a <webview>? Would using
> GuestViewBase::GetTopLevelWebContents be more appropriate? Istiaque (lazyboy@)
> might have some thoughts on this as well. I'm missing context on what the
> original problem was that this fixed.

Actually I have fixed the issue in which , 'Back', 'Forward' are not shown and
not working when pdf is embedded in the HTML page.

So we are taking owner_web_contents when guest has assiciated embedder.

But I feel GuestViewBase::GetTopLevelWebContents() is also fine, as we will get
toplevel WebContents as embedder WebContents.
I have checked using GuestViewBase::GetTopLevelWebContents(),It is working as
expected.
I am making changes as suggested.
PTAL
Thanks

[jochen (gone - plz use gerrit), 2015-05-04 07:28:56]

jochen@chromium.org changed reviewers:
+ kalman@chromium.org

[jochen (gone - plz use gerrit), 2015-05-04 07:28:57]

I wonder whether it's preferable to move the code to //extensions?

[Fady Samuel, 2015-05-04 14:17:58]

On 2015/05/04 07:28:57, jochen wrote:
> I wonder whether it's preferable to move the code to //extensions?

jochen@: The extensions module doesn't support context menus AFAIK.

[Deepak, 2015-05-05 02:36:14]

@lazyboy and @fsamuel
Please review.
Thanks

[lazyboy, 2015-05-05 05:16:16]

https://codereview.chromium.org/1117893002/diff/80001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1117893002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:310:
extensions::GuestViewBase::GetTopLevelWebContents(web_contents);
GuestViewBase is too broad for this, I'd only use
top_level_web_contents if |web_contents| is a MimeHandlerViewGuest.

i.e. guard this with "if (MimeHandlerViewGuest::FromWC(web_contents))"

[Deepak, 2015-05-05 06:28:22]

Changes done as suggested.
PTAL
Thanks

https://codereview.chromium.org/1117893002/diff/80001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1117893002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:310:
extensions::GuestViewBase::GetTopLevelWebContents(web_contents);
On 2015/05/05 05:16:16, lazyboy wrote:
> GuestViewBase is too broad for this, I'd only use
> top_level_web_contents if |web_contents| is a MimeHandlerViewGuest.
> 
> i.e. guard this with "if (MimeHandlerViewGuest::FromWC(web_contents))"

Done.

[lazyboy, 2015-05-05 06:49:54]

LGTM

https://codereview.chromium.org/1117893002/diff/100001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1117893002/diff/100001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:311: // If
we're viewing in a MimeHandlerViewGuest, use its embedder WebContents.
nit: indent and move this before the if statement below.

[Deepak, 2015-05-05 06:54:47]

Done.
PTAL

https://codereview.chromium.org/1117893002/diff/100001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1117893002/diff/100001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:311: // If
we're viewing in a MimeHandlerViewGuest, use its embedder WebContents.
On 2015/05/05 06:49:53, lazyboy wrote:
> nit: indent and move this before the if statement below.

Done.

[jochen (gone - plz use gerrit), 2015-05-05 14:01:01]

lgtm

[Deepak, 2015-05-05 14:02:32]

On 2015/05/05 14:01:01, jochen wrote:
> lgtm

@fsamuel
Please give approval for 
guest_view/web_view/context_menu_content_type_web_view.h 
guest_view/web_view/context_menu_content_type_web_view.cpp

Thanks

[Fady Samuel, 2015-05-05 14:48:10]

lgtm

[Deepak, 2015-05-05 14:49:10]

On 2015/05/05 14:48:10, Fady Samuel wrote:
> lgtm

Thanks @lazyboy,@jochen,@fsamuel.

[Deepak, 2015-05-05 14:49:17]

The CQ bit was checked by deepak.m1@samsung.com

[Deepak, 2015-05-05 14:49:17]

The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org
Link to the patchset: https://codereview.chromium.org/1117893002/#ps120001
(title: "Addressing nit.")

[commit-bot: I haz the power, 2015-05-05 14:49:48]

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

[commit-bot: I haz the power, 2015-05-05 16:35:13]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-05 16:36:16]

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