1451525 - Convert roboextender extension to a webextension

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, enhancement, P3)

Comment 1

[Andrew Swan [:aswan]]

Attachment: Bug 1451525 Convert roboextender to a webextension

Review commit: https://reviewboard.mozilla.org/r/256366/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256366/

Comment 2

[Andrew Swan [:aswan]]

Joel, I'm not sure who knows this code but the old extension manifest names you as the author.  Feel free to redirect if there's somebody else more appropriate.

You'll notice that I removed all the "scroll" event handling since nothing in-tree appears to use it.  Do we have out-of-tree robocop tests that might be broken if that is removed?  If not, this extension appears to just do two things: register a handler for the Robocop:quit message and register a few chrome resources as an elaborate way to run some test-only chrome-privileged javascript code.  It might be worth finding a way to do this without an extension at all, but converting the existing extension is the expedient route for now.

Comment 3

[Andrew Swan [:aswan]]

Nick, from comment 2, the specific questions are:
1. are there out-of-tree robocopy tests that might use the "scroll" event handling that roboextender used to do?
2. is it worth pursuing getting rid of this extension altogether?  (if-it-aint-broke-dont-fix-it is a perfectly acceptable answer)

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8991481 [details]
Bug 1451525 Convert roboextender to a webextension

https://reviewboard.mozilla.org/r/256366/#review263348

thanks for this review and porting this extension.  Do we support web extensions in fennec and geckoview?  I had thought we didn't or there were some limitations, likewise I think there were some limitations on addons so maybe this is not of concern.

I remember a perf test we turned off a couple years ago that used the scroll feature, I will also see of gbrown has concerns with this patch;  From my looking around this all looks good.

::: mobile/android/tests/browser/robocop/roboextender/schema.json:1
(Diff revision 1)
> +[]

why are we creating a file that is effectively empty?  is this required for a web extension?

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

:gbrown, do you have any concerns with this change?  You might have more context on the current state/location/future of robocop tests.  Likewise do you see any concerns with this regarding the new brave world of geckoview?

Comment 6

[Geoff Brown [:gbrown]]

I am not concerned. Robocop:Quit is important; I checked the try run and it looks fine. I am a little surprised that the scroll stuff is not in use...but only a little.

As far as I know, there is no interest in running robocop in geckoview test apps: robocop is fennec-only.

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to Andrew Swan [:aswan] from comment #3)
> Nick, from comment 2, the specific questions are:
> 1. are there out-of-tree robocopy tests that might use the "scroll" event
> handling that roboextender used to do?

Note that I'm aware of.

> 2. is it worth pursuing getting rid of this extension altogether? 
> (if-it-aint-broke-dont-fix-it is a perfectly acceptable answer)

I wanted to do this once, and per #c6, the Robocop:Quit is in fact critical.  It would not be outrageous to expose that message in some way to tests to get rid of the functionality altogether, but neither is it important.

Thanks for getting rid of this noise!

Comment 8

[Andrew Swan [:aswan]]

(In reply to Joel Maher ( :jmaher ) (UTC+2) (PTO: Back July 17) from comment #4)
> thanks for this review and porting this extension.  Do we support web
> extensions in fennec and geckoview?  I had thought we didn't or there were
> some limitations, likewise I think there were some limitations on addons so
> maybe this is not of concern.

The public webextensions APIs on fennec are not as extensive as they are on desktop, but this extension doesn't use public APIs.  This patch just moves from bootstrap.js to the webextension experiment framework since bootstrap is scheduled to be removed soon.

> ::: mobile/android/tests/browser/robocop/roboextender/schema.json:1
> (Diff revision 1)
> > +[]
> 
> why are we creating a file that is effectively empty?  is this required for
> a web extension?

Yeah, this is a little annoying.  The webextension experiments framework was designed originally for developing extension APIs.  This extension doesn't actually create an APIs (it just runs some code when the extension is loaded via the onStartup() method) but a schema declaring the extension's custom APIs is still mandatory...


As for the subsequent comments, yes Robocop:Quit is certainly important, I didn't mean to suggest we didn't need it.  But its a pretty simple bridge from Java to Services.startup, even though its test-only it could be landed directly into Fennec core.  But there's still the issue of loading privileged pages which would require tinkering with the larger test framework.  If robocop won't be used at all for GeckoView, then I think we should do the minimum here which is to just land this patch as-is and then forget all about it :)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf0fdd57920c
Convert roboextender to a webextension r=jmaher

Comment 10

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/cf0fdd57920c