Move execute_code_function to extensions.

Move execute_code_function to extensions.

BUG=352293
Committed: https://crrev.com/cc21ae467a60a64e14a1f112c496be7b65a2b5f5
Cr-Commit-Position: refs/heads/master@{#293770}

[lfg, 2014-09-04 18:31:14]

lfg@chromium.org changed reviewers:
+ rockot@chromium.org

[Ken Rockot(use gerrit already), 2014-09-04 20:51:57]

On 2014/09/04 18:31:15, lfg wrote:

Haven't done a full review yet, but for starters I think it's safe to say that
we definitely do not want any tabs-related concepts creeping into //extensions.
Tabs are strictly a chrome concept.

[Fady Samuel, 2014-09-04 20:58:24]

On 2014/09/04 20:51:57, Ken Rockot wrote:
> On 2014/09/04 18:31:15, lfg wrote:
> 
> Haven't done a full review yet, but for starters I think it's safe to say that
> we definitely do not want any tabs-related concepts creeping into
//extensions.
> Tabs are strictly a chrome concept.

I do agree that tabs_constants should probably not move. ExecuteCodeFunction is
a base class for executeScript though. WebView has one implementation and the
tabs API has another.

[Ken Rockot(use gerrit already), 2014-09-04 21:09:34]

Yeah, it seems fine to want to move ExecuteCodeFunction. Though to be
honest I am also not at all a fan of this
base-class-templated-over-another-ExtensionFunction-primitive pattern,
which I was hoping would be a one-off and potentially temporary fix for the
CaptureWebContentsFunction case. In a perfect world we wouldn't even have
*AsyncExtensionFunction anymore because UIThreadExtensionFunction has a
much cleaner interface these days.

So with that in mind, can we try to avoid that templating pattern here?
Couldn't some //chrome code provide a primitive like
ChromeExtensionFunctionDetails (which essentially does all the default
initialization and state capture that Chrome*Function is now doing), and
then that could be composed into Chrome function implementations that need
access to common Chrome data like the current profile?



On Thu, Sep 4, 2014 at 1:58 PM, <fsamuel@chromium.org> wrote:

> On 2014/09/04 20:51:57, Ken Rockot wrote:
>
>> On 2014/09/04 18:31:15, lfg wrote:
>>
>
>  Haven't done a full review yet, but for starters I think it's safe to say
>> that
>> we definitely do not want any tabs-related concepts creeping into
>>
> //extensions.
>
>> Tabs are strictly a chrome concept.
>>
>
> I do agree that tabs_constants should probably not move.
> ExecuteCodeFunction is
> a base class for executeScript though. WebView has one implementation and
> the
> tabs API has another.
>
> https://codereview.chromium.org/538063003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[lfg, 2014-09-04 21:25:40]

Yes, I think this should be possible in this case, as it only uses the Profile
from the extension function. In the capture_web_contents case though, it's a lot
harder, as it calls GetCurrentBrowser() from the (chrome) extension function.

I'll try to rewrite this patch without using the templated inheritance.


On 2014/09/04 21:09:34, Ken Rockot wrote:
> Yeah, it seems fine to want to move ExecuteCodeFunction. Though to be
> honest I am also not at all a fan of this
> base-class-templated-over-another-ExtensionFunction-primitive pattern,
> which I was hoping would be a one-off and potentially temporary fix for the
> CaptureWebContentsFunction case. In a perfect world we wouldn't even have
> *AsyncExtensionFunction anymore because UIThreadExtensionFunction has a
> much cleaner interface these days.
> 
> So with that in mind, can we try to avoid that templating pattern here?
> Couldn't some //chrome code provide a primitive like
> ChromeExtensionFunctionDetails (which essentially does all the default
> initialization and state capture that Chrome*Function is now doing), and
> then that could be composed into Chrome function implementations that need
> access to common Chrome data like the current profile?
> 
> 
> 
> On Thu, Sep 4, 2014 at 1:58 PM, <mailto:fsamuel@chromium.org> wrote:
> 
> > On 2014/09/04 20:51:57, Ken Rockot wrote:
> >
> >> On 2014/09/04 18:31:15, lfg wrote:
> >>
> >
> >  Haven't done a full review yet, but for starters I think it's safe to say
> >> that
> >> we definitely do not want any tabs-related concepts creeping into
> >>
> > //extensions.
> >
> >> Tabs are strictly a chrome concept.
> >>
> >
> > I do agree that tabs_constants should probably not move.
> > ExecuteCodeFunction is
> > a base class for executeScript though. WebView has one implementation and
> > the
> > tabs API has another.
> >
> > https://codereview.chromium.org/538063003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2014-09-04 21:27:54]

In that case, I would just use Profile::GetFromBrowserContext(context())
directly in the implementation rather than GetProfile(). I'll whip up some
composable chrome thing so we can eventually phase out Chrome*Function base
classes altogether - no need to do that here I guess.


On Thu, Sep 4, 2014 at 2:25 PM, <lfg@chromium.org> wrote:

> Yes, I think this should be possible in this case, as it only uses the
> Profile
> from the extension function. In the capture_web_contents case though, it's
> a lot
> harder, as it calls GetCurrentBrowser() from the (chrome) extension
> function.
>
> I'll try to rewrite this patch without using the templated inheritance.
>
>
>
> On 2014/09/04 21:09:34, Ken Rockot wrote:
>
>> Yeah, it seems fine to want to move ExecuteCodeFunction. Though to be
>> honest I am also not at all a fan of this
>> base-class-templated-over-another-ExtensionFunction-primitive pattern,
>> which I was hoping would be a one-off and potentially temporary fix for
>> the
>> CaptureWebContentsFunction case. In a perfect world we wouldn't even have
>> *AsyncExtensionFunction anymore because UIThreadExtensionFunction has a
>> much cleaner interface these days.
>>
>
>  So with that in mind, can we try to avoid that templating pattern here?
>> Couldn't some //chrome code provide a primitive like
>> ChromeExtensionFunctionDetails (which essentially does all the default
>> initialization and state capture that Chrome*Function is now doing), and
>> then that could be composed into Chrome function implementations that need
>> access to common Chrome data like the current profile?
>>
>
>
>
>  On Thu, Sep 4, 2014 at 1:58 PM, <mailto:fsamuel@chromium.org> wrote:
>>
>
>  > On 2014/09/04 20:51:57, Ken Rockot wrote:
>> >
>> >> On 2014/09/04 18:31:15, lfg wrote:
>> >>
>> >
>> >  Haven't done a full review yet, but for starters I think it's safe to
>> say
>> >> that
>> >> we definitely do not want any tabs-related concepts creeping into
>> >>
>> > //extensions.
>> >
>> >> Tabs are strictly a chrome concept.
>> >>
>> >
>> > I do agree that tabs_constants should probably not move.
>> > ExecuteCodeFunction is
>> > a base class for executeScript though. WebView has one implementation
>> and
>> > the
>> > tabs API has another.
>> >
>> > https://codereview.chromium.org/538063003/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/538063003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2014-09-04 21:34:04]

Also sorry, that would be Profile::FromBrowserContext(browser_context())  :)


On Thu, Sep 4, 2014 at 2:27 PM, Ken Rockot <rockot@chromium.org> wrote:

> In that case, I would just use Profile::GetFromBrowserContext(context())
> directly in the implementation rather than GetProfile(). I'll whip up some
> composable chrome thing so we can eventually phase out Chrome*Function base
> classes altogether - no need to do that here I guess.
>
>
> On Thu, Sep 4, 2014 at 2:25 PM, <lfg@chromium.org> wrote:
>
>> Yes, I think this should be possible in this case, as it only uses the
>> Profile
>> from the extension function. In the capture_web_contents case though,
>> it's a lot
>> harder, as it calls GetCurrentBrowser() from the (chrome) extension
>> function.
>>
>> I'll try to rewrite this patch without using the templated inheritance.
>>
>>
>>
>> On 2014/09/04 21:09:34, Ken Rockot wrote:
>>
>>> Yeah, it seems fine to want to move ExecuteCodeFunction. Though to be
>>> honest I am also not at all a fan of this
>>> base-class-templated-over-another-ExtensionFunction-primitive pattern,
>>> which I was hoping would be a one-off and potentially temporary fix for
>>> the
>>> CaptureWebContentsFunction case. In a perfect world we wouldn't even have
>>> *AsyncExtensionFunction anymore because UIThreadExtensionFunction has a
>>> much cleaner interface these days.
>>>
>>
>>  So with that in mind, can we try to avoid that templating pattern here?
>>> Couldn't some //chrome code provide a primitive like
>>> ChromeExtensionFunctionDetails (which essentially does all the default
>>> initialization and state capture that Chrome*Function is now doing), and
>>> then that could be composed into Chrome function implementations that
>>> need
>>> access to common Chrome data like the current profile?
>>>
>>
>>
>>
>>  On Thu, Sep 4, 2014 at 1:58 PM, <mailto:fsamuel@chromium.org> wrote:
>>>
>>
>>  > On 2014/09/04 20:51:57, Ken Rockot wrote:
>>> >
>>> >> On 2014/09/04 18:31:15, lfg wrote:
>>> >>
>>> >
>>> >  Haven't done a full review yet, but for starters I think it's safe to
>>> say
>>> >> that
>>> >> we definitely do not want any tabs-related concepts creeping into
>>> >>
>>> > //extensions.
>>> >
>>> >> Tabs are strictly a chrome concept.
>>> >>
>>> >
>>> > I do agree that tabs_constants should probably not move.
>>> > ExecuteCodeFunction is
>>> > a base class for executeScript though. WebView has one implementation
>>> and
>>> > the
>>> > tabs API has another.
>>> >
>>> > https://codereview.chromium.org/538063003/
>>> >
>>>
>>
>>  To unsubscribe from this group and stop receiving emails from it, send an
>>>
>> email
>>
>>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>>
>>
>>
>>
>> https://codereview.chromium.org/538063003/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[lfg, 2014-09-04 22:41:21]

PTAL, I've rebased against HEAD + https://codereview.chromium.org/543873002/ .


On 2014/09/04 21:34:04, Ken Rockot wrote:
> Also sorry, that would be Profile::FromBrowserContext(browser_context())  :)
> 
> 
> On Thu, Sep 4, 2014 at 2:27 PM, Ken Rockot <mailto:rockot@chromium.org> wrote:
> 
> > In that case, I would just use Profile::GetFromBrowserContext(context())
> > directly in the implementation rather than GetProfile(). I'll whip up some
> > composable chrome thing so we can eventually phase out Chrome*Function base
> > classes altogether - no need to do that here I guess.
> >
> >
> > On Thu, Sep 4, 2014 at 2:25 PM, <mailto:lfg@chromium.org> wrote:
> >
> >> Yes, I think this should be possible in this case, as it only uses the
> >> Profile
> >> from the extension function. In the capture_web_contents case though,
> >> it's a lot
> >> harder, as it calls GetCurrentBrowser() from the (chrome) extension
> >> function.
> >>
> >> I'll try to rewrite this patch without using the templated inheritance.
> >>
> >>
> >>
> >> On 2014/09/04 21:09:34, Ken Rockot wrote:
> >>
> >>> Yeah, it seems fine to want to move ExecuteCodeFunction. Though to be
> >>> honest I am also not at all a fan of this
> >>> base-class-templated-over-another-ExtensionFunction-primitive pattern,
> >>> which I was hoping would be a one-off and potentially temporary fix for
> >>> the
> >>> CaptureWebContentsFunction case. In a perfect world we wouldn't even have
> >>> *AsyncExtensionFunction anymore because UIThreadExtensionFunction has a
> >>> much cleaner interface these days.
> >>>
> >>
> >>  So with that in mind, can we try to avoid that templating pattern here?
> >>> Couldn't some //chrome code provide a primitive like
> >>> ChromeExtensionFunctionDetails (which essentially does all the default
> >>> initialization and state capture that Chrome*Function is now doing), and
> >>> then that could be composed into Chrome function implementations that
> >>> need
> >>> access to common Chrome data like the current profile?
> >>>
> >>
> >>
> >>
> >>  On Thu, Sep 4, 2014 at 1:58 PM, <mailto:fsamuel@chromium.org> wrote:
> >>>
> >>
> >>  > On 2014/09/04 20:51:57, Ken Rockot wrote:
> >>> >
> >>> >> On 2014/09/04 18:31:15, lfg wrote:
> >>> >>
> >>> >
> >>> >  Haven't done a full review yet, but for starters I think it's safe to
> >>> say
> >>> >> that
> >>> >> we definitely do not want any tabs-related concepts creeping into
> >>> >>
> >>> > //extensions.
> >>> >
> >>> >> Tabs are strictly a chrome concept.
> >>> >>
> >>> >
> >>> > I do agree that tabs_constants should probably not move.
> >>> > ExecuteCodeFunction is
> >>> > a base class for executeScript though. WebView has one implementation
> >>> and
> >>> > the
> >>> > tabs API has another.
> >>> >
> >>> > https://codereview.chromium.org/538063003/
> >>> >
> >>>
> >>
> >>  To unsubscribe from this group and stop receiving emails from it, send an
> >>>
> >> email
> >>
> >>> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>>
> >>
> >>
> >>
> >> https://codereview.chromium.org/538063003/
> >>
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2014-09-04 22:54:47]

lgtm pending landing of base CL

https://codereview.chromium.org/538063003/diff/80001/extensions/browser/api/e...
File extensions/browser/api/execute_code_function.cc (right):

https://codereview.chromium.org/538063003/diff/80001/extensions/browser/api/e...
extensions/browser/api/execute_code_function.cc:33: const std::string& data) {
nit: alignment

https://codereview.chromium.org/538063003/diff/80001/extensions/browser/api/e...
extensions/browser/api/execute_code_function.cc:49: &extension_default_locale);
nit: alignment

running "git cl format" makes life easier most of the time.

https://codereview.chromium.org/538063003/diff/80001/extensions/common/api/ex...
File extensions/common/api/extension_types.json (right):

https://codereview.chromium.org/538063003/diff/80001/extensions/common/api/ex...
extensions/common/api/extension_types.json:31: "id": "InjectDetails",
Awesome. This seems like a perfectly reasonable home for this type.

https://codereview.chromium.org/538063003/diff/80001/extensions/common/consta...
File extensions/common/constants.cc (right):

https://codereview.chromium.org/538063003/diff/80001/extensions/common/consta...
extensions/common/constants.cc:61: const char kMoreThanOneValuesError[] = "Code
and file should not be specified "
Could you fix this constant name now that it's moving to general extensions
constants? How about kCodeAndFileNotAllowedError or something. And maybe the
text could be clarified "... should not both be specified."


Actually, on second look, are any of these constants referenced outside of
execute_code_function.cc? If not, please just move them into that file in an
anonymous namespace.

[Fady Samuel, 2014-09-04 22:59:58]

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

[Fady Samuel, 2014-09-04 22:59:59]

https://codereview.chromium.org/538063003/diff/80001/extensions/common/consta...
File extensions/common/constants.cc (right):

https://codereview.chromium.org/538063003/diff/80001/extensions/common/consta...
extensions/common/constants.cc:61: const char kMoreThanOneValuesError[] = "Code
and file should not be specified "
On 2014/09/04 22:54:47, Ken Rockot wrote:
> Could you fix this constant name now that it's moving to general extensions
> constants? How about kCodeAndFileNotAllowedError or something. And maybe the
> text could be clarified "... should not both be specified."
> 
> 
> Actually, on second look, are any of these constants referenced outside of
> execute_code_function.cc? If not, please just move them into that file in an
> anonymous namespace.

I don't want to block this CL but should these strings be internationalized?

[Ken Rockot(use gerrit already), 2014-09-04 23:01:27]

Generally we don't bother localizing console error strings. There are tons
of these strings throughout the code and localization would be of
questionable value given the audience.


On Thu, Sep 4, 2014 at 3:59 PM, <fsamuel@chromium.org> wrote:

>
> https://codereview.chromium.org/538063003/diff/80001/
> extensions/common/constants.cc
> File extensions/common/constants.cc (right):
>
> https://codereview.chromium.org/538063003/diff/80001/
> extensions/common/constants.cc#newcode61
> extensions/common/constants.cc:61: const char kMoreThanOneValuesError[]
> = "Code and file should not be specified "
> On 2014/09/04 22:54:47, Ken Rockot wrote:
>
>> Could you fix this constant name now that it's moving to general
>>
> extensions
>
>> constants? How about kCodeAndFileNotAllowedError or something. And
>>
> maybe the
>
>> text could be clarified "... should not both be specified."
>>
>
>
>  Actually, on second look, are any of these constants referenced
>>
> outside of
>
>> execute_code_function.cc? If not, please just move them into that file
>>
> in an
>
>> anonymous namespace.
>>
>
> I don't want to block this CL but should these strings be
> internationalized?
>
> https://codereview.chromium.org/538063003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[lfg, 2014-09-05 14:41:02]

On 2014/09/04 22:54:47, Ken Rockot wrote:
> lgtm pending landing of base CL
> 
>
https://codereview.chromium.org/538063003/diff/80001/extensions/browser/api/e...
> File extensions/browser/api/execute_code_function.cc (right):
> 
>
https://codereview.chromium.org/538063003/diff/80001/extensions/browser/api/e...
> extensions/browser/api/execute_code_function.cc:33: const std::string& data) {
> nit: alignment
> 
>
https://codereview.chromium.org/538063003/diff/80001/extensions/browser/api/e...
> extensions/browser/api/execute_code_function.cc:49:
&extension_default_locale);
> nit: alignment
> 
> running "git cl format" makes life easier most of the time.

Done. I just forgot to re-run it after the last change :(

>
https://codereview.chromium.org/538063003/diff/80001/extensions/common/api/ex...
> File extensions/common/api/extension_types.json (right):
> 
>
https://codereview.chromium.org/538063003/diff/80001/extensions/common/api/ex...
> extensions/common/api/extension_types.json:31: "id": "InjectDetails",
> Awesome. This seems like a perfectly reasonable home for this type.
> 
>
https://codereview.chromium.org/538063003/diff/80001/extensions/common/consta...
> File extensions/common/constants.cc (right):
> 
>
https://codereview.chromium.org/538063003/diff/80001/extensions/common/consta...
> extensions/common/constants.cc:61: const char kMoreThanOneValuesError[] =
"Code
> and file should not be specified "
> Could you fix this constant name now that it's moving to general extensions
> constants? How about kCodeAndFileNotAllowedError or something. And maybe the
> text could be clarified "... should not both be specified."
> 
> 
> Actually, on second look, are any of these constants referenced outside of
> execute_code_function.cc? If not, please just move them into that file in an
> anonymous namespace.

They are only used by this file, so I moved them to an anonymous namespace.

[Fady Samuel, 2014-09-05 14:43:00]

lgtm

[lfg, 2014-09-08 16:07:31]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2014-09-08 16:09:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/lfg@chromium.org/538063003/160001

[littleone691_gmail.com, 2014-09-08 16:10:45]

thanks




On Mon, Sep 8, 2014 at 12:09 PM, <commit-bot@chromium.org> wrote:

> CQ is trying da patch. Follow status at
>  https://chromium-status.appspot.com/cq/lfg@chromium.org/538063003/160001
>
>
> https://codereview.chromium.org/538063003/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Extensions reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to extensions-reviews+unsubscribe@chromium.org.
> To view this discussion on the web visit https://groups.google.com/a/
> chromium.org/d/msgid/extensions-reviews/90e6ba3fcdd7f5ea940502900acf%
> 40google.com.
>

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, 2014-09-08 19:36:28]

Committed patchset #9 (id:160001) as 03ae304424c8f04a7a790b70a2e4a97cf814cb22

[commit-bot: I haz the power, 2014-09-10 03:47:41]

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