Closed Bug 641829 Opened 9 years ago Closed 6 years ago

Remove enablePrivilege from profileserver.py's index.html

Categories

(Firefox Build System :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla26

People

(Reporter: ted, Assigned: emk)

References

()

Details

Attachments

(1 file, 2 obsolete files)

The index.html file we use for PGO profiling runs uses enablePrivilege so it can quit the browser. We'll need to remove that. Joel said he was going to add a quit() method to SpecialPowers, so we should probably just fix profileserver.py to add SpecialPowers to the profile it uses, and then use that.
this is not as simple as it should be.  Curretly profileServer.py uses automation.initializeProfile (which is good), but mochitest handles the special powers (code and profile insertion) in the runtests.py script.

In order for this to happen, we need to:
 - get special powers out of mochitest (either just for this test, or relocate it in the build tree)
 - add code to automation.py or somewhere to setup special powers so we don't duplicate code.
Duplicate of this bug: 657973
I don't really care either way. This literally just needs a quit method, so even making a duplicate seriously stripped down SpecialPowers or something with just that one method would be fine.
Assignee: nobody → ted.mielczarek
Status: NEW → ASSIGNED
This patch adds a stripped-down copy of SpecialPowers to tools/quitter that exposes just one function to content: Quitter.quit(). It felt a bit like overkill, but so did moving SpecialPowers around just to add a single method for this to use. I'm not sure if this is the best solution, but it's not terrible. I haven't tested this in Fennec yet, but I think it ought to work fine.
Attachment #535317 - Flags: review?(jmaher)
Doesn't seem to work in Fennec for some reason. Extension is installed properly but the object isn't available in content... Might spend a little time tracking that down since I don't want to have to fix it in the future.
Comment on attachment 535317 [details] [diff] [review]
add a Quitter extension for profileserver.py

Review of attachment 535317 [details] [diff] [review]:
-----------------------------------------------------------------

this seems pretty reasonable.  I personally don't like adding yet another addon, but this seems like the most reasonable approach with what we have now.  I just want to make sure that we are only building the quitter xpi for PGO builds and other builds that use profileserver.py.

::: toolkit/toolkit-tiers.mk
@@ +266,5 @@
>  tier_platform_dirs	+= tools/codesighs
>  endif
>  
> +tier_platform_dirs	+= tools/quitter
> +

does this build quitter all the time?  don't we want it for pgo builds only?
Attachment #535317 - Flags: review?(jmaher) → review+
We use profileserver for the Valgrind builds as well, I'm not sure if they're a specific build configuration I can test for.
Yeah, that. I think we should probably just build this unconditionally. It's only one extra directory. Since we copy profileserver.py around unconditionally, it would be weird to have some supporting machinery for it not get built in certain configurations.
Any reason is this patch not landed yet?
Blocks: 911573
I don't remember why. It probably needs to be rebased at this point.
Attached patch quitter (obsolete) — Splinter Review
Rebased
Attachment #535317 - Attachment is obsolete: true
Attachment #803674 - Flags: review?(ted)
Comment on attachment 803674 [details] [diff] [review]
quitter

Review of attachment 803674 [details] [diff] [review]:
-----------------------------------------------------------------

I can't really review my own patch, but if all you've done is rebased it then you can carry forward r=jmaher from before. Have you run this past the Try server yet, or should I?

::: tools/quitter/Makefile.in
@@ +6,5 @@
> +DEPTH           = ../..
> +topsrcdir       = @top_srcdir@
> +srcdir          = @srcdir@
> +VPATH           = @srcdir@
> +relativesrcdir  = tools/quitter

This boilerplate is no longer necessary.
Attachment #803674 - Flags: review?(ted)
Attached patch quitter v1.1Splinter Review
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13)
> I can't really review my own patch, but if all you've done is rebased it
> then you can carry forward r=jmaher from before.

Hm, I had to add some nontrivial changes, so requesting :jmaher for review again.

> Have you run this past the
> Try server yet, or should I?

Running on try: https://tbpl.mozilla.org/?tree=Try&rev=3cde93da5b04

> ::: tools/quitter/Makefile.in
> @@ +6,5 @@
> > +DEPTH           = ../..
> > +topsrcdir       = @top_srcdir@
> > +srcdir          = @srcdir@
> > +VPATH           = @srcdir@
> > +relativesrcdir  = tools/quitter
> 
> This boilerplate is no longer necessary.

Removed.
Attachment #803674 - Attachment is obsolete: true
Attachment #804049 - Flags: review?(jmaher)
(In reply to Masatoshi Kimura [:emk] from comment #14)
> Running on try: https://tbpl.mozilla.org/?tree=Try&rev=3cde93da5b04

According to bug 811404, the OS X error is ignorable.
Comment on attachment 804049 [details] [diff] [review]
quitter v1.1

Review of attachment 804049 [details] [diff] [review]:
-----------------------------------------------------------------

please test this on try server for all platforms, OSX does some weird stuff.

::: toolkit/toolkit.mozbuild
@@ +180,5 @@
>  
>  if CONFIG['ENABLE_MARIONETTE'] or CONFIG['MOZ_WIDGET_TOOLKIT'] not in ('gonk', 'android'):
>      add_tier_dir('platform', 'testing/marionette')
>  
> +add_tier_dir('platform', 'tools/quitter')

do we want to add this for all builds?

::: tools/quitter/install.rdf
@@ +4,5 @@
> +     xmlns:em="http://www.mozilla.org/2004/em-rdf#">
> +
> +  <Description about="urn:mozilla:install-manifest">
> +    <em:id>quitter@mozilla.org</em:id>
> +    <em:version>2011.05.25</em:version>

might as well update this date :)

@@ +7,5 @@
> +    <em:id>quitter@mozilla.org</em:id>
> +    <em:version>2011.05.25</em:version>
> +    <em:type>2</em:type>
> +
> +    <!-- Target Application this extension can install into, 

nit: trailing whitespace

@@ +21,5 @@
> +    <!-- Front End MetaData -->
> +    <em:name>Quitter</em:name>
> +    <em:description>Adds a quit method that content pages can use to quit the application.</em:description>
> +    <em:creator>Mozilla</em:creator>
> +  </Description>      

nit: trailing whitespace

::: tools/quitter/jar.mn
@@ +1,3 @@
> +quitter.jar:
> +% content quitter %content/
> +  content/contentscript.js (contentscript.js)

do you ned the QuitterObserver.js at all?
Attachment #804049 - Flags: review?(jmaher) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/182cb77cc3e9

(In reply to Joel Maher (:jmaher) from comment #16)
> please test this on try server for all platforms, OSX does some weird stuff.

Done. https://tbpl.mozilla.org/?tree=Try&rev=a7c18f96a50f
We don't support PGO on OSX (at least officially).
http://hg.mozilla.org/build/buildbot-configs/file/97caef8e4fd6/mozilla/config.py#l73

> ::: toolkit/toolkit.mozbuild
> @@ +180,5 @@
> >  
> >  if CONFIG['ENABLE_MARIONETTE'] or CONFIG['MOZ_WIDGET_TOOLKIT'] not in ('gonk', 'android'):
> >      add_tier_dir('platform', 'testing/marionette')
> >  
> > +add_tier_dir('platform', 'tools/quitter')
> 
> do we want to add this for all builds?

See comment #9.

> ::: tools/quitter/jar.mn
> @@ +1,3 @@
> > +quitter.jar:
> > +% content quitter %content/
> > +  content/contentscript.js (contentscript.js)
> 
> do you ned the QuitterObserver.js at all?

Yes. It's listed in chrome.manifest.
Assignee: ted → VYV03354
https://hg.mozilla.org/mozilla-central/rev/182cb77cc3e9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.