1475503 - Stop using preprocessor in reftest.jsm

Status: RESOLVED FIXED

(Testing :: Reftest, enhancement)

Description

[Marco Castelluccio [:marco]]

Rebuilding just to pick up changes in reftest.jsm is painful. The preprocessor is only used in two places, perhaps we can stop using it.

> #ifdef MOZ_ENABLE_SKIA_PDF

We can move this to AppConstants.

> #ifdef XP_MACOSX
>     try {
>         var dock = Cc["@mozilla.org/widget/macdocksupport;1"].getService(Ci.nsIMacDockSupport);
>         dock.activateApplication(true);
>     } catch(ex) {
>     }
> #endif

We don't need this, since we're eating the exception anyways.

Comment 1

[Andrew Halberstadt [:ahal]]

+1

Though fyi you can run |./mach build layout/tools/reftest| (which takes ~2 seconds) in case you weren't already.

Comment 2

[nikshepsvn]

willing to take this on if you can give me a little more context

Comment 3

[Marco Castelluccio [:marco]]

(In reply to nikshepsvn from comment #2)
> willing to take this on if you can give me a little more context

Sure, thanks!

In layout/tools/reftest/reftest.jsm, you have to remove the preprocessor usages (the ones I outlined in comment 0).
To remove the first, you'll need to create a new MOZ_ENABLE_SKIA_PDF key in toolkit/modules/AppConstants.jsm (look at the file for other similar examples).
The second preprocessor usage can simply be completely removed, as we have a try catch anyway, it means we won't do anything on Mac anyway.

Once you're done with these two changes, you can remove the "*" before reftest.jsm in layout/tools/reftest/jar.mn, which means the preprocessor won't be used anymore for this file.

Then, you can do the same treatment for layout/tools/reftest/manifest.jsm.

Before and after every step, make sure you can still run a reftest. E.g. `./mach test accessible/tests/crashtests/471493.xul`.

I'm going to be on PTO from tomorrow, so you can ask :ahal for review.

Comment 4

[Sahil Bhosale]

I would like to work on this issue but can i do this on Ubuntu? 

As I have searched for #ifdef XP_MACOSX preprocessor in AppConstants.jsm file but didn't find that is this because I am on Ubuntu?

Comment 5

[Sahil Bhosale]

Oh, I got that method in reftest.jsm file. Can I start working on it now?

Comment 6

[nikshepsvn]

Hey I'm currently working on this and almost done. I

Comment 7

[Sahil Bhosale]

But my patch is ready and i am about to submit

Comment 8

[Sahil Bhosale]

Attachment: tip.patch

Changes made and submitted the patch file

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8999326 [details] [diff] [review]
tip.patch

Apologies for the confusion here. Nikshepsvn asked first and was in the middle of working on this, so they have priority. Nikshepsvn, can you confirm that you are still planning to submit a patch?

Sahil, if I don't hear back from nikshepsvn by the start of next week I'll assign the bug to you. Though fyi this patch doesn't apply cleanly and we don't want to completely remove the call to `doc.activateApplication`.

Comment 10

[Andrew Halberstadt [:ahal]]

Apologies for the confusion here, we should have set an assignee quicker. Nikshepsvn asked first and was in the middle of working on this, so he gets priority. Nikshepsvn, can you confirm that you are still planning to submit a patch?

Sahil, if I don't hear back from nikshepsvn by the end of the week I'll assign the bug to you and review your patch.

Comment 11

[nikshepsvn]

Hey Andrew,

How's it going! Thank you for reacognizing that I started working on this, I really appreciate it. I've mostly finished the patch but I'm outside right now so I'll test and upload it as soon as I get back home. Thanks again!

Comment 12

[Sahil Bhosale]

(In reply to Andrew Halberstadt [:ahal] from comment #10)
> Apologies for the confusion here, we should have set an assignee quicker.
> Nikshepsvn asked first and was in the middle of working on this, so he gets
> priority. Nikshepsvn, can you confirm that you are still planning to submit
> a patch?
> 
> Sahil, if I don't hear back from nikshepsvn by the end of the week I'll
> assign the bug to you and review your patch.

Ok Andrew and sorry nikshepsvn for submitting the patch before you.
I started working on patch without getting the confirmation from the mentor and submitted the patch.

I apologise for that.

Comment 13

[Marco Castelluccio [:marco]]

(In reply to nikshepsvn from comment #11)
> Hey Andrew,
> 
> How's it going! Thank you for reacognizing that I started working on this, I
> really appreciate it. I've mostly finished the patch but I'm outside right
> now so I'll test and upload it as soon as I get back home. Thanks again!

nikshepsvn are you still working on this or should Sahil take over?

Comment 14

[nikshepsvn]

forgive me for the delay, will upload it today by 11:00PM EST. thanks for the patience btw

Comment 15

[nikshepsvn]

Attachment: fix.patch

Comment 16

[nikshepsvn]

just submitted the patch @marco, testing is buggy for my locally, but is this what you're expecting? 
when i try testing locally I get this message ` know you are trying to run a crashtest test. Unfortunately, I can't run those`

Comment 17

[Marco Castelluccio [:marco]]

You can use `./mach crashtest XXX` instead of `./mach test XXX`.

Comment 18

[Marco Castelluccio [:marco]]

Comment on attachment 9002992 [details] [diff] [review]
fix.patch

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

::: layout/tools/reftest/manifest.jsm
@@ -464,1 @@
>          sandbox.AndroidVersion = sysInfo.getPropertyAsInt32("version");

The same applies to this file.

::: layout/tools/reftest/reftest.jsm
@@ -267,4 @@
>          g.compareRetainedDisplayLists = prefs.getBoolPref("reftest.compareRetainedDisplayLists");
>      } catch (e) {}
>  
> -#ifdef MOZ_ENABLE_SKIA_PDF

We don't want to remove the code altogether, we want to just stop using the preprocessor (#ifdef).
Since we create a MOZ_ENABLE_SKIA_PDF variable in AppConstants.jsm, you can use that one here with a normal if.

@@ -580,4 @@
>  {
>      var fm = Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager);
>      fm.focusedWindow = g.containingWindow;
> -#ifdef XP_MACOSX

Here we won't need to use AppConstants, we can just always run the code since the catch is not doing anything and the try block doesn't have any effect on non macOS platforms.

::: toolkit/modules/AppConstants.jsm
@@ +340,5 @@
>  #else
>      false,
>  #endif
> +
> +  MOZ_ENABLE_SKIA_PDF:

In this file (AppConstants.jsm), which is still preprocessed, we only want to define these variables, so we can use them in reftest.jsm and manifest.jsm.
Moving the entire code from reftest.jsm and manifest.jsm here won't work.

Comment 19

[Sahil Bhosale]

Can I work on this??

Comment 20

[Andrew Halberstadt [:ahal]]

Sure, looks like the other contributor hasn't responded to the review feedback. Feel free to ask for help if you get stuck. Thanks!

Comment 21

[Sahil Bhosale]

Attachment: Bug 1475503 Stop using preprocessor in reftest.jsm

Comment 22

[sandhusimran3]

Attachment: Bug 1475503 Stop using preprocessor in reftest.jsm

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 25

[Sanket Markan]

Hi @marco, is this bug still open and if so, can I submit a patch?

Comment 26

[Marco Castelluccio [:marco]]

Hello Sanket, sure, feel free to work on it!

Comment 27

[Shazib Summar [:ssummar]]

Hello, I am an outreachy applicant and want to work on this. Can you please assign this to me? Thanks.

Comment 28

[Marco Castelluccio [:marco]]

Hello Shazib, feel free to work on this bug. Bugs are assigned when there is a patch to fix them.

Comment 29

[Shazib Summar [:ssummar]]

Attachment: Bug 1475503 - Removed preprocessor use in reftest.jsm. r=marco

Comment 30

[Shazib Summar [:ssummar]]

Hi again, I submitted a patch. Hopefully I didnt miss anything. Please feel free to comment on it.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/50ba34a0c174
Removed preprocessor use in reftest.jsm. r=marco DONTBUILD

Comment 32

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/50ba34a0c174