Refactor repository to have 'a' prefix on the parameters name

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: AndreeaMatei, Assigned: Kyle Colle-Tripp)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(firefox32 wontfix, firefox33 fixed, firefox34 fixed, firefox-esr31 wontfix)

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(4 attachments, 7 obsolete attachments)

(Reporter)

Description

5 years ago
In order to be consistent, we should have all parameters declared like: aController instead of controller, aSpec instead of spec and so on.
(Reporter)

Updated

5 years ago
Whiteboard: [mentor=andreea.matei][lang=js][good first bug]
(Assignee)

Comment 1

5 years ago
Hi, I'm completely new here but would like to learn how to get started. This seems like a simple edit and I'd be happy to contribute, but have no idea how. Any chance you could give me a hand learning the ropes?
(Reporter)

Comment 2

5 years ago
Hi Kyle, thanks for your interest! Sure I can show you how to get started. 
First, you will need to install Mozmill: 
https://developer.mozilla.org/en-US/docs/Mozmill

After that, please check this link on how to get our repository and run tests:
https://developer.mozilla.org/en-US/docs/Mozmill_Tests

Indeed, this bug needs all parameters names to start with 'a', I think this will help you to get familiarized with our code. Please remember that the jsdoc will also need updating, if the parameter's name is mentioned there.

Let me know if you have questions, you can also join #automation on IRC to chat more.
(In reply to Andreea Matei [:AndreeaMatei] from comment #2)
> Hi Kyle, thanks for your interest! Sure I can show you how to get started. 
> First, you will need to install Mozmill: 
> https://developer.mozilla.org/en-US/docs/Mozmill

We should not reference this link. It's outdated and should most likely be removed. Kyle, I would suggest you download the already pre-configured mozmill-environment from the website below, which only needs to be unzipped. There are no setup hassles to master. It's way easier.

https://mozqa.com/mozmill-env/

I will also assign you to that bug. Oh, and thank you for your interest. We are looking forward!
Assignee: nobody → kcolletripp
Status: NEW → ASSIGNED
(Assignee)

Comment 4

5 years ago
Thanks Henrik, turns out that Andreea's link lead eventually to your link, so I had already installed that :)

While I'm thanking people, I'd like to thank the both of you for helping me get on my feet, it's very much appreciated. Sorry if I end up asking tedious or obvious questions.

As for how to proceed, I'm guessing by Andreea's second link, that I need to install Mercurial, clone the repository and then somehow edit the files to include the 'a' prefix on the parameters (which I'll probably need help identifying), then I'm guessing I need to save these changes somewhere and upload them for review. The first two portions, the installation and cloning I can probably figure out, but the rest I'm gonna have to ask for help on. If I've missed a step please let me know!

So my first question is likely how do I open/edit the relevant files?
(Reporter)

Comment 5

5 years ago
(In reply to Henrik Skupin (:whimboo) from comment #3) 
> > https://developer.mozilla.org/en-US/docs/Mozmill
> 
> We should not reference this link. It's outdated and should most likely be
> removed.
I have updated the link last night, I think the documentation there is correct now.

(In reply to Kyle Colle-Tripp from comment #4)
> So my first question is likely how do I open/edit the relevant files?

You will need to look in the files, giving that the names are different you can't really search for them.
Our repository has the following structure:
- data folder contains local pages (html) or other types of files (video, addons) that we use to test something
- lib folder has all the libraries with different methods (you will find a lot of parameters here)
- tests - well, this is pretty obvious, there are several subdirectories there for different categories of tests - also a lot of parameters here
- templates - some examples

I don't know if you have a favorite editor, but you can use whatever you like to edit those .js files (from Text Editor in linux to Notepad++ and so on) and just add the "a" prefix (followed by capital letter) to any parameter you see. I for example use Sublime.

After you finish editing all files, I'll guide you on how to create the patch to upload here for review.
(In reply to Kyle Colle-Tripp from comment #4)
> As for how to proceed, I'm guessing by Andreea's second link, that I need to
> install Mercurial, clone the repository and then somehow edit the files to

No, Mercurial is already pre-installed. You only have to setup your local .hgrc and clone the tests as described here: https://developer.mozilla.org/en-US/docs/Mozmill_Tests#Configuring_Mercurial

I would suggest you read through the whole document to get familiar with any aspect on working on those files and the patch/review process. Andreea can indeed help you if you have troubles.
(Assignee)

Comment 7

5 years ago
As I'm going around making these small edits, what do I do in cases where there are two different parameters; one called something like 'actual' and the other labeled 'aActual', both with different values?

As well, I've run into a case where somebody had a large block of code and used the variables a and b as parameters. Would you like me to change that to aA and aB?

Thank you two so much for your help so far, I'm really learning a lot!
(Reporter)

Comment 8

5 years ago
I assume you're talking about assertions. 'actual' and 'aActual' are not in the same method I assume, so it's not affecting us to change that 'actual' in 'aActual' as well. Same for 'expected'.
Regarding 'a' and 'b', we can call them something like 'aObject1' and 'aObject2'.
(Assignee)

Comment 9

5 years ago
That's right, I was talking about assertions. Unfortunately, the variables I had previously mentioned are being called at the same time. For example, the 'expected' situation:

  if (typeof aExpected === 'string') {
    aMessage = expected;
    aExpected = null;
  }

As for the 'a' and 'b' variables, I'll get that done, thanks.
(Assignee)

Comment 10

5 years ago
Hi Andreea, any news on how I should proceed?
(Reporter)

Comment 11

5 years ago
Hi Kyle,
So I see this case here, right?
http://hg.mozilla.org/qa/mozmill-tests/file/8521d0f4c211/lib/assertions.js#l448

Hm, now that I look more at the code, I think there's a mistake there and 'expected' should have been 'aExpected', like here:
https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/assertions.js#L451

Could you please correct that?
Other than that I see only parameters that will get the 'a' prefix, if you have some other cases please give me the link. Thanks!
(Assignee)

Comment 12

5 years ago
I have a few other examples, but I have a flu so I can't really work this week.

Sorry
(Assignee)

Comment 13

5 years ago
Was looking over dom-utils.js briefly (this flu is making this difficult) and I can't really figure out the root/aRoot set-up they have there. I'm not skilled enough to fully understand this, but as I see it, "root' itself is a node being used as a parameter, and at some point this happens:

function nodeCollector(aRoot) {
  this.root = aRoot;
}

I know the 'this' keyword operates differently than 'self' like it does in most other languages, but I don't fully understand it. I'm fairly certain this is calling on the "root" node, however, and I'm not sure how I'd rename the original node "root" in this case, or even if I should.
(Reporter)

Comment 14

5 years ago
In that method you don't need to change it, it's the constructor, this.root is calling this one:
http://hg.mozilla.org/qa/mozmill-tests/file/default/lib/dom-utils.js#l527 

In that file I see most changes are required in DOMWalker class, just where you see functions with parameters that don't have 'a'. A simple replace will be enough, shouldn't be any conflicts.

Where you see 'self', is a substitute for 'this', just that if we have:
function () {
  this is here
  self = this;
  assert.something(function () {
    this will not work so we use self
  });
}

Hope you'll feel better soon!
(Assignee)

Comment 15

5 years ago
Ah ok, thanks for that explanation!

Just so we're clear, cases like this: 

walk : function DOMWalker_walk(aIds, root, aWaitFunction) {
    if (typeof aWaitFunction == 'function')
      expect.waitFor(aWaitFunction());

    if (!root)
      root = this._controller.window.document.documentElement;

    var resultsArray = this._walk(root);

[....]

Don't need "root" renamed, despite being a parameter, because it's a constructor? Speaking of which, I always thought constructors were functions and unless I'm mistaken, it appears to be a variable.

Thanks for your kind wishes! I really do hope to get better over the weekend, this headache is something else :(
(Reporter)

Comment 16

5 years ago
No, there's a parameter and it needs renaming. What I was explaining above was over the code you showed and clearing up the this/self issue. That whole function from comment 13 was the constructor, not the parameter aRoot. Giving that there it already had 'a', you didn't need to change it. And this.root is not a parameter.

To sum up, wherever you see a function with parameters that don't have 'a', you edit it. Everywhere, no exception. :)
(Assignee)

Comment 17

5 years ago
There we go, refactored all the parameters as shown, with the exception of the 'root' case we discussed earlier, since you told me not to touch that.

So what do I do from here?
(Reporter)

Comment 18

5 years ago
That's great news Kyle! Well, you need to create a patch in that repository you worked on, like this:
hg qnew patchName
hg add (you do this only if you have created new files in the repository, not the case now)
hg refresh -m "Bug 883120 - Refactor repository to have 'a' prefix on the parameters name. r=amatei" (meaning Bug XXXXX - description of what you did. r=reviewer name - we use that short)
hg export tip > patchName.patch

If after creating the patch you have to do more changes and update it, you'll just hg qrefresh and export it.

Please be sure your patch applies cleanly, giving that you've got your repository a while ago and I'm not sure you have updated it since. You an do that by cloning a clean repo and applying your patch:
hg qimport path%to%patch
hg qpush

Or in the same repository by:
hg qpop (this will pop your patch out from the repo)
hg pull -u (this will update your repo)
hg qpush (this applies your patch back)

If you get conflicts, you need to solve them manually and update the patch as explained above.

As always, find me on IRC if something is unclear. Thanks!
(Assignee)

Comment 19

5 years ago
Sorry for the delay, been dealing with rather sensitive personal matters recently. Question though, do I update the repository first, then create the patch, then upload it? Or do I create the patch, the update, then upload?
(Reporter)

Comment 20

5 years ago
You create the patch, then update, apply patch and if no conflicts upload. If conflicts (.rej files), you manually solve them and refresh the patch to upload.
You can't update the repo first cause you'll lose your changes.
(Assignee)

Comment 21

5 years ago
Ah ok, figured as much, but wanted to be sure. Thanks for the help :)
(Assignee)

Comment 22

5 years ago
Alright, I just uploaded the patch, it appears to have applied cleanly, although I'm not sure how to verify this. Let me know how I did :)
Hi Kyle, can I ask where you have uploaded the patch? I cannot find it attached to this bug. Went something wrong?
(Assignee)

Comment 24

5 years ago
It didn't give me any error messages or anything, truth be told I'm not entirely sure where it uploaded to, I simply followed the directions, and called the patch "refactorPatch."

I have the patch created and the repository updated, would you like me to export it again? If so, the command is 'hg export tip > refactorPatch.patch' correct?
(Reporter)

Comment 25

5 years ago
So you created the patch in your local repository.

For the conflicts part, you need to clone another repository and run:
hg qimport path_to_the_patch
hg qpush

If it applies cleanly, we're all good.

For us to see it you need to upload it here in the bug, at the top you'll find "Add an attachment", give the path to the patch, a name (like patch v1) check patch and ask review (at the review line, flag "?" and add my name, you'll get the full contact as a result).
(Assignee)

Comment 26

5 years ago
Created attachment 778075 [details] [diff] [review]
Refactored repository patch v1
Attachment #778075 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #778075 - Flags: review? → review?(andreea.matei)
(Reporter)

Comment 27

5 years ago
Kyle, somehow your patch is empty, has only the header. 
In the repository where you have changed the files, if you run hg qdiff do you get the changes in the console?
If yes, then run a hg qrefresh, export it and check it by opening it in a editor. You should see all the changes with + and - and it's good for upload here.
(Reporter)

Updated

5 years ago
Attachment #778075 - Attachment is obsolete: true
Attachment #778075 - Flags: review?(andreea.matei)
(Assignee)

Comment 28

5 years ago
I don't get anything when I enter that command. I tried it in every folder that contained files I changed.
(Reporter)

Comment 29

5 years ago
Hm, that's strange. So you can still see the changes in one file for example? What does hg qapplied return? I hope we can catch each other on IRC to nail this down. Until then, here's a useful link on how to work with patches:

https://developer.mozilla.org/en-US/docs/Mercurial_Queues
(Assignee)

Comment 30

5 years ago
I've checked a bunch of the files and the changes themselves are still there, even after updating the repo.

hg qapplied returns: "refactorPatch"

You're in the #automation room, correct?
(Assignee)

Comment 31

5 years ago
Created attachment 781734 [details] [diff] [review]
Refactored repository patch v2

This patch actually has content. I know the header is missing, I was wondering how to get the command shell to apply the 'git' format automatically.
Attachment #781734 - Flags: review?(andreea.matei)
(Reporter)

Comment 32

5 years ago
Comment on attachment 781734 [details] [diff] [review]
Refactored repository patch v2

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

Thank you so much for your work! I can see the huge amount of time spent on this. It took me a while to review it, there are some things to fix and hopefully the next round will be final and we can land this :)

It would be great if you could run at least a functional testrun with your patch applied, to check that tests are passing, cause now some failed. 
http://mozmill-crowd.blargon7.com/#/functional/report/b3aefd29a5c41bda1cc43616d3c9bea1

You can find here how to do that:
https://developer.mozilla.org/en-US/docs/Mozmill_Tests#Running_Mozmill_tests

I'd also recommend you to have different patches for each folder. It's easier for both of us, especially if the repository changes and you have to solve merge conflicts.
You can have a patch with changes in lib folder only, then in tests/functional, another for tests/l10n and so on.
Let me know if you have questions! Thanks!

::: lib/assertions.js
@@ +102,5 @@
> +    return _deepEqual(aObject1, aObject2);
> +  }
> +  try {
> +    var ka = Object.keys(aObject1),
> +        kb = Object.keys(aObject2),

I would change this ka and kb as well with kObject1 and kObject2, since those are now a and b.

::: lib/dom-utils.js
@@ +184,4 @@
>     *                                          elements will be opened
>     *                                          [1|2|4]
>     *                          title         - title of the opened dialog/window
> +   *                          aWaitFunction  - The function used as an argument

I don't think this needs to be changed as it's not a parameter in this method.

@@ +190,5 @@
>     *                                          [optional - default: no waiting]
>     *                          windowHandler - Window instance
>     *                                          [only needed for some tests]
>     *
> +   * @returns aNode

Same here.

@@ +225,4 @@
>     *                                          elements will be opened
>     *                                          [1|2|4]
>     *                          title         - title of the opened dialog/window
> +   *                          aWaitFunction  - The function used as an argument

Same here.

@@ +263,4 @@
>                                  callbackFilter :  this._callbackFilter,
>                                  callbackNodeTest : this._callbackNodeTest,
>                                  callbackResults : this._callbackResults,
> +                                aWaitFunction : idSet.aWaitFunction}

And here.

@@ +291,4 @@
>                                              this._callbackResults);
>                domWalker.walk(idSet.subContent,
>                               controller.window.document.documentElement,
> +                             idSet.aWaitFunction);

This will not be necessary after leaving waitFunction as it was.

@@ +345,5 @@
>  
>          // If the target is a new modal/non-modal window, this.walk() has to be
>          // started by the method opening that window. If not, we do it here.
> +        if (aIdSet.target == DOMWalker.WINDOW_CURRENT)
> +          this.walk(aIdSet.subContent, null, aIdSet.aWaitFunction);

waitFunction here

@@ +348,5 @@
> +        if (aIdSet.target == DOMWalker.WINDOW_CURRENT)
> +          this.walk(aIdSet.subContent, null, aIdSet.aWaitFunction);
> +      }
> +      else if (nodeToProcess.selected && aIdSet.subContent &&
> +                 aIdSet.subContent.length > 0) {

Could you please move this line 2 spaces to the left so it's aligned with nodeToProcess?

@@ +371,5 @@
>  
>          // If the target is a new modal/non-modal window, this.walk() has to be
>          // started by the method opening that window. If not, we do it here.
> +        if (aIdSet.target == DOMWalker.WINDOW_CURRENT)
> +          this.walk(aIdSet.subContent, null, aIdSet.aWaitFunction);

waitFunction here.

@@ +389,5 @@
>  
>          // If the target is a new modal/non-modal window, this.walk() has to be
>          // started by the method opening that window. If not, we do it here.
> +        if (aIdSet.target == DOMWalker.WINDOW_CURRENT)
> +          this.walk(aIdSet.subContent, null, aIdSet.aWaitFunction);

waitFunction here.

::: lib/downloads.js
@@ +128,4 @@
>     */
>    cancelActiveDownloads : function downloadManager_cancelActiveDownloads() {
>      // Get a list of all active downloads (nsISimpleEnumerator)
> +    var aDownloads = Services.downloads.activeDownloads;

This is not a parameter in this method and has nothing to do with the one in other methods that is actual parameter.

@@ +401,3 @@
>   * automatically to disk
>   *
>   * @param {MozMillController} controller MozMillController of the browser window

This is also aController

::: lib/software-update.js
@@ +567,3 @@
>     *        Mozmill controller of the browser window
>     */
> +  waitForDialogOpen : function softwareUpdate_waitForDialogOpen(aBrowserController) {

Hm, I see this parameter is not used in the method at all. I guess it should have been instead of this._controller. Could you please file a bug under Other Products -> Mozilla QA -> Mozmill Tests to have this updated? Thanks!

::: lib/tests/testSessionStore.js
@@ +19,1 @@
>  }

controller should be aModule.controller since we declared it like that.

::: lib/utils.js
@@ +211,4 @@
>                                                       searchTerm, submitButton,
>                                                       timeout) {
> +  aController.waitThenClick(searchField, timeout);
> +  aController.type(searchField, searchTerm);

These are also parameters, searchField, searchTerm, submitButton and timeout.

@@ +354,2 @@
>   *        Specifies how to check for the new window (possible values: type or title)
> + * @param {string} aType

This is aText, aType was above :)

@@ +360,4 @@
>   *        Make sure the window is closed after the return from the callback handler
>   * @returns The MozMillController of the window (if the window hasn't been closed)
>   */
> +function handleWindow(aType, aType, aCallback, aClose) {

Same here, please check this whole method.

::: templates/testPreferencesDialog.js
@@ +14,4 @@
>  
>  // Run the preferences dialog test
>  var testSampleTestcase = function() {
> +  prefs.openPreferencesDialog(aController, callbackHandler);

This is not a parameter in this method, it will not work. In the one bellow it is so it works fine there.
Attachment #781734 - Flags: review?(andreea.matei) → review-
(Assignee)

Comment 33

5 years ago
Before I get to work on those changes, I've got two quick question. First of all, how do I make a patch that's focused on one folder only?

Second, do you have any other bugs that you could point me towards, preferably with you as a mentor, that I could work on? I'd like to try writing a line of code and you've been a great mentor, so I thought I'd ask you.

While it may have taken some time, it was a lot of fun :) I'd attribute that largely to your patience with me.
(Reporter)

Comment 34

5 years ago
Thanks Kyle, sure we have more bugs! :)

Here's a simple one with only one change needed: bug 844787. It will help you to get used to the patches.

Hm, to make patches for each folder.. it's complicated now that all are changed. You could as a workaround just clone a repository and overwrite a modified folder from the other repo and then create the patch. Each folder would need it's own repository.

If it's too complicated, continue as it is now, just be sure to do testruns so you don't create failures. Thanks!
(Assignee)

Comment 35

5 years ago
I've made my changes and updated my patch, unfortunately I can't find you on IRC and I'm having a few problems getting my tests to run. Specifically, when I run the functional test, it says no builds have been specified. Any guidance here would be useful.

Also, for the time being, I'm going to leave the patch as one giant mess, since I can't for the life of me figure out how to make the multiple patches. Sorry about that :(

Out of curiosity, when is usually the best time to find you on IRC, and in what time zone? I've tried a few times now and I've yet to find you. Your IRC name is AndreeaMatei in the #automation channel, correct?
(Assignee)

Comment 36

5 years ago
Oh, when filing the bug, I go under other options>Mozilla QA. When it get there, it just asks me for a description of the bug, rather than any sort of prompt for mozmill tests. In fact, I can't find mozmill tests under the other options list at all. How should I proceed?

Sorry to double post, I know it's tacky, but I don't know how to edit my previous post.
(Reporter)

Comment 37

5 years ago
No problem Kyle, it's fine with one patch only as well.
If you have it exported, make a sanity check that it's still applying (we're changing the repository almost daily):
hg qpop -> will pop you patch
hg pull -u -> this will update the repository
hg qpush -> this is pushing back your patch against the latest repository. If you get conflicts over some files, it means those were changed and your current patch didn't had those changes. You have to update them manually. You'll get a .rej file for each conflict which shows you what didn't apply. After updating manually, hg qrefresh and export in the same patch file.

For functional testrun, you probably didn't pass the nightly build:
hg clone http://hg.mozilla.org/qa/mozmill-automation - if you've done this already, ignore
cd mozmill-automation
./testrun_functional.py --repository=path_to_your_changed_repository* --report=http://mozmill-crowd.blargon7.com/db path_to_nightly_exe**

*the repository has to have the patch exported, otherwise the testrun won't recognize your changes.
** for exemple in ubuntu, lets say you have the build in nightly folder, you go nightly/firefox/firefox (the application)

On IRC I'm from 9-10 AM until 5-6 PM, EET time. You can also email me if it's easier: andreea.matei@softvision.ro

Regarding the bug, I see you filed it already. The Mozmill Tests is in the menu, above the description :)
(Assignee)

Comment 38

5 years ago
I filed the bug, apparently it had already been filed, despite my checking for something similar. Oh well, it's been taken care of at least.

As for the patch, it applied cleanly, but I'm still having trouble getting the tests to run. I kept getting errors regarding my path until I tried this:

$ ./testrun_functional.py --repository=C:\\Users\\Kyle\\mozmill-tests --report=http://mozmill-crowd.blargon7.com/db C:\\Users\\Kyle\\mozilla-central\\obj-i686-pc-mingw32\\browser\\app\\firefox.exe

At which point this was returned:

*** No builds have been specified. Use --help to see all options.

I haven't been able to actually resolve this though, so I still can't run the tests. 
The path to the nightly build needed the double '\' in order to not give me a path error, but the first path 'C:\\Users\\Kyle\\mozmill-tests' didn't seem to change whether I used double '\' or not. Not sure if this is relevant information, but figured I'd include it.

Also, I work on EST, so by the time you start I'm asleep, and when I start you're logging off.
(Reporter)

Comment 39

5 years ago
Kyle, please try with this build:
ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-central/

There you can find the latest nightly build and then everytime you need to use it, you can just update it through Help - About Nightly. We have updates daily.
(Assignee)

Comment 40

5 years ago
Alright, managed to get the tests to run!
Unfortunately, when I follow the link provided I get:

{"error":"not_found","reason":"Document is missing attachment"}

The link was: http://mozmill-crowd.blargon7.com/dbb7ef1fb3d9703aeaf2c46e07d20230e1

I'm going to try another one, see if anything changes.
(In reply to Kyle Colle-Tripp from comment #40)
> Alright, managed to get the tests to run!
> Unfortunately, when I follow the link provided I get:
> 
> {"error":"not_found","reason":"Document is missing attachment"}
> 
> The link was:
> http://mozmill-crowd.blargon7.com/dbb7ef1fb3d9703aeaf2c46e07d20230e1
> 
> I'm going to try another one, see if anything changes.

You're missing a trailing slash:
http://mozmill-crowd.blargon7.com/db/b7ef1fb3d9703aeaf2c46e07d20230e1
http://mozmill-crowd.blargon7.com/#/functional/report/b7ef1fb3d9703aeaf2c46e07d20230e1
(Assignee)

Comment 42

5 years ago
Oh, ok. Thanks for clearing that up. So... what do I do from here?
(Reporter)

Comment 43

5 years ago
You need to fix the errors you got there, cause those are caused by the changes in your patch. Track where you use mozmill.aController and leave it as it was, mozmill.controller.

Also something in the opening method from addons.js is wrong. If you hover the error message, you'll get a notification with some more details, code lines from addons.js to help track it.

Great that you were able to run this and caught it in time, before you attached the patch!
(Assignee)

Comment 44

5 years ago
Hey there Andreea, sorry I've been a bit out of touch. Been dealing with university matters. I've solved a few problems, but created a few more. The lastest batch of tests yielded this: http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a16d8103 so I'm now looking into what on earth tab.controller is, since I can't find any instances of it. I'm hoping it's a controller variable in tabs.js.

Just so you know, since I'll be in university starting Monday, I won't be able to work on this as frequently, but I'll try and post an update at minimum of once a month, I'd like to aim for once a week. Figured I'd give you a heads up here.
(Reporter)

Comment 45

5 years ago
Hi Kyle,
I will need to see your changed patch to help identify where the tab.controller error is. I see we have one "tab.controller" in our repository, here:
http://hg.mozilla.org/qa/mozmill-tests/file/default/lib/addons.js#l197
But that method doesn't need any changes, neither the other methods called in it.
(Reporter)

Comment 46

4 years ago
Hi Kyle,
Would you still be interested to continue the work on this bug?
(Assignee)

Comment 47

4 years ago
It certainly has been far too long. I haven't lost interest, instead I've lost time. What I would like to do is start from scratch, since I ran into so many errors on my last run-through, and this time around I can actually identify what it is I need to change.

I'm up to my ears in work, but this saturday I'll be entirely free, which is when i intend on hammering this out. Could you be so kind as to tell me how to update my build and delete my patch, so I can start a new one?
(Reporter)

Comment 48

4 years ago
Thanks for getting back on us Kyle! Well, a lot has happened since then here too, our repository structure is changed, we've devided it into firefox and metrofirefox for the new tests we're developing for metro. I would suggest you clone a new repository and just start from scratch. 
The other way would be hg qpop your current patch, hg qdelete patch_name, hg pull -u to update your repository and then start from scratch :). 

Let me know if you have difficulties.
(Assignee)

Comment 49

4 years ago
Alright, ran into a few issues, but I think I managed to get the test repository updated and I see what you mean by the new structure.

Just a few questions to help jog my memory, in the .../mozmill-tests folder, which subfolders should I be making changes to? Which kinds of files should I be making changes to (.html, .js, etc). And would you like me to make changes to the parameters of both the definitions and calls of functions (and classes, if I see any?)

Sorry this came out late, was putting together a stack adt for unviersity to solve a few simple problems. On the bright side, if ever you folks need to know how to re-arrange the order of the cars in a trainyard I'm your man.
(Reporter)

Comment 50

4 years ago
In all folders with lib and tests, I don't think data folder will have any instances, so only .js.
Only defined parameters in functions and classes, not the calls. I mean cases like controller.click(moreInfoButton); won't need any change.
The libraries will need most of the changes.

Thanks!
(Reporter)

Comment 51

4 years ago
Hi Kyle! I imagine you have a lot of work at the university, do you think you'll have time to finish up this bug? Thanks.
(Assignee)

Comment 52

4 years ago
Hey Andreea,

I've been chipping away at it slowly but surely. I'd reckon I've got about half of all the files looked at, those being the bigger ones. I've only found times Monday afternoons to actually sit down and work on it, but this week in particular I'm hoping I have a bit of extra time. Sorry for not keeping you up to date.
(Assignee)

Comment 53

4 years ago
Well, out of school for a while, so I should be able to finish this quickly.

I've made all my changes, I'm just having trouble testing them.

I updated my patch, updated my repo, exported it, went into mozmill-automation and ran

$ ./testrun_functional.py --repository=C:\\Users\\Kyle\\mozmill-tests --report=http://mozmill-crowd.blargon7.com/db C:\\Program\ Files\ \(x86\)\\Nightly

Which produced the following: http://pastebin.com/RLfJQiFC

There's no mention of which file the error might have stemmed from.

Even more interesting, my nightly build no longer exists. Twice now has it deleted my nightly build.

When it hits an error (*** Installing crashreporter.exe => c:\users\kyle\appdata\local\temp\tmpsof_ve.binary\) a popup says it has no idea what's wrong and so it can't report the error.

Sorry for being out of the loop for so long, but on the bright side, I'm here daily for 4~ months, so I should be able to power through this. Thanks for putting up with that :)

I could always update the patch if you want me to. Chances are something's wrong though.
Can you please tell us which command line you are using to actually run the testrun script? Something is broken with that.
(Assignee)

Comment 55

4 years ago
I'm using the one in mozmill-env.
Sure, but which options do you specify. Please tell us the full command line.
(Assignee)

Comment 57

4 years ago
Mercurial Distributed SCM (version 2.1)

My mercurial.ini file looks like this:

[ui]
username = Kyle Colle-Tripp <kcolletripp@gmail.com>
merge = internal:merge

[diff]
git = 1
showfunc = 1
unified = 8

[defaults]
qnew = -U

[extensions]
hgext.color =
hgext.mq =
hgext.transplant =
mq =

[hooks]
pretxncommit.whitespace = hg export tip | (! egrep '^\+(.*[ ]*|[\t]*)$')
prechangegroup.mq-no-pull = ! hg qtop > /dev/null 2>&1


I get this feeling my command line may be out of date, I'll try updating it tomorrow.
Kyle, I'm not asking you about the content of the mercurial configuration, but I would like to know how you start a testrun. So like 'testrun_functional %path_to_firefox% --option1 --option2 ...'
(Reporter)

Comment 59

4 years ago
Kyle, I'm wondering if you have the latest env / mozmill, from the command you used in comment 53, it looks like you have your own mozmill-automation clone, not using the one included in mozmill-env:
http://mozqa.com/mozmill-env - get the latest for your platform and then you can run with:

testrun_functional.py --repository=C:\\Users\\Kyle\\mozmill-tests --report=http://mozmill-crowd.blargon7.com/db C:\\Program\ Files\ \(x86\)\\Nightly

Should work then.
(Assignee)

Comment 60

4 years ago
Hey there, sorry about the delay, was a national holiday.

So I updated my mozmill-env to 2.0.6, I just took the latest version. However, I can't for the life of me figure out how to update my mozmill-automation. When I try and run the above commnand from my (outdated) mozmill-automation it gives me an error saying it can't find some files.

Now, you said that there should be a mozmill-automation clone included the the mozmill-env file, but I don't see it. I've looked through both the page and the file and I see nothing of the sort.

I also ran "$ hg pull -u" from my mozmill-automation folder and it had 1 change in 1 file. Again, it didn't work when I tried to run the tests.

The last thing I've tried is looking at this: https://github.com/mozilla/mozmill-automation/
It seems to be the 'new' mozmill automation. It says to install the new mozmill-automation, I have to run "python setup.py develop" but I'm not sure from what directory. Heck, my mozmill shell won't even let me delete the $, so I'm not sure how to run that command at all.

I feel so close, yet so far :(
(Reporter)

Comment 61

4 years ago
Kyle, as you had the mozmill-automation cloned before, you ran a testrun with ./testrun... 

Now, in mozmill-env console, just type testrun_remote and the path to firefox (and other options like repository, report if you want), and it will work.

Even when you type testrun_ and hit the tab key, should already give you suggestions.
(Assignee)

Comment 62

4 years ago
Well, I tried "$ testrun_remote C:\\Program\ Files\ \(x86\)\\Nightly" and it gave me an error as specified in the following document: http://pastebin.com/UQwFnMW1

Same thing happens when I try to run any test.

Which I believe is the same error as before, only this time it doesn't delete my nightly build.
(Reporter)

Comment 63

4 years ago
I don;t understand why it takes crashreporter.exe, so the command should be:
testrun_remote --repository=path/to/your/repo /path/to/firefox.exe/file

Please try to download the .zip archive from here and unzip it and pass the path to the .exe file in your command. Lets see if that works.

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/ - the zip for your platform I believe should be this one?

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/firefox-32.0a1.en-US.win32.zip
(In reply to Kyle Colle-Tripp from comment #62)
> Well, I tried "$ testrun_remote C:\\Program\ Files\ \(x86\)\\Nightly" and it
> gave me an error as specified in the following document:

What you simply have to run is:

"$ testrun_remote C:\\Program\ Files\ \(x86\)\\Nightly\\firefox.exe"

Regarding the crashreporter.exe file, it looks like we pick the first executable of that folder. I filed the following issue for mozmill-automation: https://github.com/mozilla/mozmill-automation/issues/149
(Assignee)

Comment 65

4 years ago
Thanks Henrik, but I followed Andreea's comment and got it to work!

The results are here: 
http://mozmill-crowd.blargon7.com/#/functional/report/7a69d3e8424ecf8642bb6566e4bb2d0a

I'll get to fixing the errors.


For future reference, the command that worked was 
$ testrun_remote --repository=C:\\Users\\Kyle\\mozmill-tests --report=http://mozmill-crowd.blargon7.com/db C:\\Program\ Files\ \(x86\)\\firefox\\firefox.exe
(In reply to Kyle Colle-Tripp from comment #65)
> For future reference, the command that worked was 
> $ testrun_remote --repository=C:\\Users\\Kyle\\mozmill-tests
> --report=http://mozmill-crowd.blargon7.com/db C:\\Program\ Files\
> \(x86\)\\firefox\\firefox.exe

Well, that's a different build. Here you execute a probably release build instead of the Nightly, as what you tried formerly.
(Reporter)

Comment 67

4 years ago
The build I gave above was also for nightly.
(Assignee)

Comment 68

4 years ago
http://mozmill-crowd.blargon7.com/#/functional/report/7a69d3e8424ecf8642bb6566e4d0e658

And there we have it. Updated the repo, no conflicts. I'll go ahead and upload it.
(Assignee)

Comment 69

4 years ago
Created attachment 8426474 [details] [diff] [review]
bug883120 - Refactored repository.patch
Attachment #781734 - Attachment is obsolete: true
(Assignee)

Comment 70

4 years ago
Hey Andreea, if I promise to finish it in a timely manner, could you direct me to another bug?
(Reporter)

Comment 71

4 years ago
Hi Kyle. Yeah sure, we'll find you a new one once this is done. You can also join our Automation training days, tomorrow and next Wednesday on #automation, if you have the time :)
(Assignee)

Comment 72

4 years ago
Thanks Andreea. Out of curiosity, what time are they?
(Reporter)

Comment 73

4 years ago
Will be around all day for Europe time and also a bit covering US time, most likely the first part of the day for US. But you can stop by anytime on #automation and say hi to us :)
(Assignee)

Comment 74

4 years ago
Hey there Andreea, was just wondering how the review was coming along?

Haven't heard from you in a while.
Comment on attachment 8426474 [details] [diff] [review]
bug883120 - Refactored repository.patch

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

Kyle, when you upload a patch which is ready for review, please do not forget to set the review flag. Otherwise it might be missed, as happened here.
Attachment #8426474 - Flags: review?(andreea.matei)
(Assignee)

Comment 76

4 years ago
My apologies, I tried to set it but I must have done it incorrectly.
(Reporter)

Comment 77

4 years ago
Comment on attachment 8426474 [details] [diff] [review]
bug883120 - Refactored repository.patch

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

Uh, sorry I missed this, from the last comments I haven't seen the patch, understood you will work soon on it.

Overall looks good, we just need to change the name in the jsdoc as well and please add a commit message with:
hg qrefresh -m "Bug number - description. r=amatei"

I ran remote testrun and have it passing, the functional one failed due to the mutation parameter. Please run testruns as well to make sure you haven't miss anything, it's a large patch :) Thanks!

::: firefox/lib/addons.js
@@ +1628,4 @@
>   *        as documented at https://developer.mozilla.org/en/Addons/Add-on_Manager/Addon
>   *        and returns a filtered version.
>   */
> +function getInstalledAddons(aCallbackFilter) {

Please also update the parameter name in the jsdoc above. Same for all cases.

::: firefox/lib/localization.js
@@ +216,4 @@
>        // Code specific to the preferences panes to reject out not visible nodes
>        // in the panes.
> +      if (aNode.parentNode && (aNode.parentNode.localName == "prefwindow" &&
> +                              aNode.parentNode.currentPane.id != aNode.id) ||

1 space shifted to the right please

@@ +218,5 @@
> +      if (aNode.parentNode && (aNode.parentNode.localName == "prefwindow" &&
> +                              aNode.parentNode.currentPane.id != aNode.id) ||
> +          ((aNode.parentNode.localName == "tabpanels" ||
> +            aNode.parentNode.localName == "deck") &&
> +           aNode.parentNode.selectedPanel.id != aNode.id)) {

Same here.

::: firefox/lib/tabs.js
@@ +25,5 @@
>    init: function (aElement) {
>      var win = aElement.getNode().ownerDocument.defaultView;
>      var self = this;
> +    this.mutationObserver = new win.MutationObserver(function (aMutations) {
> +      aMutations.forEach(function (aMutations) {

This is 'aMutation' and below the same at 'mutation'.
Attachment #8426474 - Flags: review?(andreea.matei) → review-

Updated

4 years ago
Mentor: andreea.matei
Whiteboard: [mentor=andreea.matei][lang=js][good first bug] → [lang=js][good first bug]
(Assignee)

Comment 78

4 years ago
Hey, sorry this is taking so long. My parents split up recently and it's been tough. That, and my right wrist being inflamed makes it hard to type. But, I have everything but the jsdoc finished and I'll upload the patch (with fixed documentation) in probably two days.

Thanks for sticking with me here, I do appreciate it.
(Assignee)

Comment 79

4 years ago
Created attachment 8453985 [details] [diff] [review]
bug883120.patch

Ran functional and remote testruns, everything checked out fine. Should be up to date.
Attachment #8426474 - Attachment is obsolete: true
Attachment #8453985 - Flags: review+
Attachment #8453985 - Flags: feedback+
(Assignee)

Comment 80

4 years ago
Created attachment 8453988 [details] [diff] [review]
bug883120.patch

Sorry, forgot to properly flag the last patch. This one should be correct.
Attachment #8453985 - Attachment is obsolete: true
Attachment #8453988 - Flags: review?(andreea.matei)

Comment 81

4 years ago
Comment on attachment 8453988 [details] [diff] [review]
bug883120.patch

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

There's a couple of conflicts.
To easily update just pop the patch from the queue, update the repo, push the patch again:
> hg qpop
> hg pull -u
> hg qpush

You will see some messages like:
> Hunk #1 succeeded at 56 with fuzz 2 
These will be fixed by the next qrefresh. Mercurial was able to apply the changes.

Besides those, there are 2 conflicts, and mercurial will generate a .rej file for each of them:
> data/geolocation/position.html.rej
> firefox/lib/utils.js.rej
Check the rej and the original file, reapply your changes (you can delete the rej file afterwards).

After you finished with this, run `hg qrefresh` and rexport the patch.

::: firefox/lib/downloads.js
@@ +151,1 @@
>     * @param {Array of download} downloads

I see you updated _some_ jsdoc comments.
Please check and update the name of the variable in the JSdoc comment for all rename instances.

::: firefox/lib/prefs.js
@@ +112,5 @@
>     *        (Optional) If true the OK button is clicked on Windows which saves
>     *        the changes. On OS X and Linux changes are applied immediately
>     */
> +  close : function preferencesDialog_close(aSaveChanges) {
> +    aSaveChanges = (aSaveChanges == undefined) ? false : aSaveChanges;

In cases like this we have a local variable which gets its value from an argument.
So the line could have been left:
> var saveChanges = (aSaveChanges == undefined) ? false : aSaveChanges;
ANd you wouldn't need to make updates in the rest of the function.

::: firefox/lib/search.js
@@ +908,3 @@
>    }
>  };
>  

There are a few of setters in this file which you have missed, namely:
> set selectedEngine(name) {
> set selectedIndex(index) {
> set suggestionsEnabled(state) {
> set enginesDropDownOpen(newState) {
> set selectedEngine(name) {

::: firefox/lib/software-update.js
@@ +348,5 @@
>     * @param {number} timeout
>     *        Timeout the download has to stop
>     */
> +  download : function softwareUpdate_download(aChannel, aWaitForFinish, aTimeout) {
> +    aWaitForFinish = aWaitForFinish ? aWaitForFinish : true;

I would leave this as:
> var waitForFinish = aWaitForFinish || true;

Then you can leave the original `waitForFinish` variable in the rest of the function.

@@ +551,5 @@
>     * Wait that check for updates has been finished
>     * @param {number} timeout
>     */
> +  waitForCheckFinished : function softwareUpdate_waitForCheckFinished(aTimeout) {
> +    aTimeout = aTimeout ? aTimeout : TIMEOUT_UPDATE_CHECK;

Same here:
> var timeout = aTimeout || TIMEOUT_UPDATE_CHECK;

@@ +583,5 @@
>     * @param {number} timeout
>     *        Timeout the download has to stop
>     */
> +  waitforDownloadFinished: function softwareUpdate_waitForDownloadFinished(aTimeout) {
> +    aTimeout = aTimeout ? aTimeout : TIMEOUT_UPDATE_DOWNLOAD;

And same here:
> var timeout = aTimeout || TIMEOUT_UPDATE_DOWNLOAD;

::: firefox/lib/tabs.js
@@ +714,3 @@
>  
>      // Get the tab panel and check if an element has to be fetched
>      var panel = this.getElement({type: "tabs_tabPanel", subtype: "tab", value: this.getTab(index)});

In this file you missed a setter function:
> set selectedIndex(index) {

::: firefox/tests/endurance/testBookmarks_OpenAndClose/test1.js
@@ +1,1 @@
> +  /* This Source Code Form is subject to the terms of the Mozilla Public

nit: I think you accidentally left a space here
Attachment #8453988 - Flags: review?(andreea.matei) → review-
(Assignee)

Comment 82

4 years ago
Created attachment 8454519 [details] [diff] [review]
bug883120.patch

I goofed, didn't hg qrefresh -> hg export after updating yesterday. On the bright side, I managed to get about half the changes Andrei suggested alongside a ton more. Anyways, this one applies cleanly and it passed both tests without problems.
Attachment #8453988 - Attachment is obsolete: true
Attachment #8454519 - Flags: review?(andreea.matei)
(Assignee)

Comment 83

4 years ago
I just realized, I meant to say that I had gotten half of Andrei's suggestions yesterday. All of his suggestions are included in this patch.
(Reporter)

Comment 84

4 years ago
Comment on attachment 8454519 [details] [diff] [review]
bug883120.patch

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

Thanks Kyle for the patch, I was in PTO last week but I see Andrei guided you.
We have a few changes left, I did testruns for all to make sure all pass, unfortunately addons testrun is skipped now because of the selenium addon. With these fixed, we can land it :)

::: firefox/lib/tabview.js
@@ +619,3 @@
>                }
>              default:
>                return true;

In this file we miss a aNode parameter to the "tabs" case. Please see:
http://mozmill-crowd.blargon7.com/#/endurance/report/db75145408bff5ccf8a072085b005b9c

::: lib/dom-utils.js
@@ +15,2 @@
>   *        MozMill controller of the window to operate on.
> + * @param {Function} CallbackFilter

aCallbackFilter please

@@ +24,5 @@
> +function DOMWalker(aController, aCallbackFilter, aCallbackNodeTest,
> +                   aCallbackResults) {
> +
> +  this._controller = aController;
> +  this._callbackFilter = CallbackFilter;

Same here.

::: lib/graphics.js
@@ +203,5 @@
>    /**
>     * Push graphics feature info
>     */
> +  _pushFeatureInfo : function Graphics_pushFeatureInfo(aName, aFeature, aIsEnabled, aMessage) {
> +    Message = Message || aIsEnabled;

This is aMessage actually, same below.
Attachment #8454519 - Flags: review?(andreea.matei) → review-
(Assignee)

Comment 85

4 years ago
Created attachment 8460720 [details] [diff] [review]
Bug 883120 patch

Updated and tested, came out clean. Caught a few other things that should have been changed, but nothing major.
Attachment #8454519 - Attachment is obsolete: true
Attachment #8460720 - Flags: review?(andreea.matei)
(Assignee)

Comment 86

4 years ago
Hi Andreea,

As I was updating my repository, I noticed that there had been a number of updates since I uploaded the patch. There was one message staying that: "Hunk #1 succeeded at 64 with fuzz 2 (offset 0 lines)"

I know this means I don't have to make any changes on my end, but does that mean I need to re-upload my patch, or is it alright as is?
(Reporter)

Comment 87

4 years ago
Hi Kyle. The fuzz line it's ok, not affecting the landing. I'll review this today, thanks!
(Reporter)

Comment 88

4 years ago
Comment on attachment 8460720 [details] [diff] [review]
Bug 883120 patch

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

This looks good and tested well (remote, functional, endurance). I'll land it in the morning to be able to check results quickly on production, it's a big patch :) Thanks Kyle!
Attachment #8460720 - Flags: review?(andreea.matei)
Attachment #8460720 - Flags: review+
Attachment #8460720 - Flags: checkin?(andreea.matei)
(Reporter)

Comment 89

4 years ago
Comment on attachment 8460720 [details] [diff] [review]
Bug 883120 patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/2dd1fca1e94a (default)

Thanks Kyle, please let me know if you're interested on other bugs I could recommend you.
Attachment #8460720 - Flags: checkin?(andreea.matei) → checkin+
(Reporter)

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox34: --- → fixed
Resolution: --- → FIXED
(Assignee)

Comment 90

4 years ago
Hi Andreea,

I'd appreciate it if you could direct me towards any other bugs. You've been a great mentor, and I'd like to continue working with you.

Glad to see this thing finally finished!
(Reporter)

Comment 91

4 years ago
Kyle, great! Lets check in bug 1000832 :) Please have a look and feel free to assign and ask questions if something is unclear there.
The patch on this bug is massive and will most likely cause problems with backports of other patches. So I would propose that we get this patch backported down to 31.0esr. What do you think Andreea?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 93

4 years ago
+1 from me to get this backported.
(Reporter)

Comment 94

4 years ago
Yeah, we could do that. Kyle, can you check this? But also, keep in mind the fix from bug 1047349, it was a failure from here and we needed fix quickly so we've done it via that bug.

We want to include it in the new patch for aurora and all the other branches.

So you just need a clean repo and:
hg up -C mozilla-aurora
hg qimport the_patch

Solve conflicts manually if needed (go in the files affected and check the changes that didn't apply) and also add the change in bug 1047349.

Then hg qexport and add the patch here :)
Thanks!
(Assignee)

Comment 95

4 years ago
Created attachment 8468002 [details] [diff] [review]
aurora patch

Hi Andreea,

I've made the requested changes, and there were no problems aside from some fuzz that was handled automatically. I handled the change in bug 1047349. That part was fine.

Before making any of the prior changes, I updated my repo and reapplied my patch. There were a ton of failures. However, when I looked over the .rej files, there didn't seem to be any problems in the files themselves, just changes that mostly included removing sections. I ran functional and remote tests and everything worked just fine. Just figured I'd let you know.

Anyways, here's the patch.
Attachment #8460720 - Attachment is obsolete: true
Attachment #8468002 - Flags: review?(andreea.matei)
(Reporter)

Updated

4 years ago
Attachment #8460720 - Attachment is obsolete: false
(Reporter)

Comment 96

4 years ago
Comment on attachment 8468002 [details] [diff] [review]
aurora patch

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

Thanks, this is working fine. Lets check the other branches too, maybe we're in luck and it applies cleanly :)
http://hg.mozilla.org/qa/mozmill-tests/rev/c44c2f2cda1a (aurora)

I removed the obsolete for the other patch, for default, as we don't obsolete final patches that get checked in.
Attachment #8468002 - Flags: review?(andreea.matei) → review+
(Reporter)

Updated

4 years ago
Attachment #8468002 - Attachment description: bug883120.patch → aurora patch
(Reporter)

Updated

4 years ago
status-firefox31: --- → affected
status-firefox32: --- → affected
status-firefox33: --- → fixed
status-firefox-esr31: --- → affected
(Assignee)

Comment 97

4 years ago
Ah ok, sorry about the obsolete thing. Thanks for telling me though.
Depends on: 1054189
Hi Kyle, can you please make sure to also include the bustage fix from bug 1054189 into the backport patch for beta? Thanks.
Status: REOPENED → ASSIGNED
(Assignee)

Comment 99

4 years ago
Sure, a little busy this weekend, but I'll get it done.
(Assignee)

Comment 100

4 years ago
Hey, so I took a look at the line he mentioned, but it seems as though it's fine.

In my repo, the line is: notAllowedLocalNames.indexOf(aNode.localName) == -1) {

However, in the bug, the line looks like: notAllowedLocalNames.indexOf(node.localName) == -1) {

I see what the problem would be, but I don't see how he has it. If I didn't make this change, somebody else already did. I followed Andreea's steps in comment 94. Did you make this change already, or am I missing something?

Comment 101

4 years ago
(In reply to Kyle Colle-Tripp from comment #100)
> Hey, so I took a look at the line he mentioned, but it seems as though it's
> fine.
> 
> In my repo, the line is: notAllowedLocalNames.indexOf(aNode.localName) ==
> -1) {
> 
> However, in the bug, the line looks like:
> notAllowedLocalNames.indexOf(node.localName) == -1) {
> 
> I see what the problem would be, but I don't see how he has it. If I didn't
> make this change, somebody else already did. I followed Andreea's steps in
> comment 94. Did you make this change already, or am I missing something?

Well yes, Henrik fixed the issue in bug 1054189.

What's still left to do here is to backport the changes to the remaining branches:
mozilla-beta, mozilla-release, mozilla-esr31

Comment 98 informed that when we do that, to make sure the fix from bug 1054189 is also included.
(Assignee)

Comment 102

4 years ago
Ah ok, so I'd just need to run the following commands:

hg up -C mozilla-aurora
hg qimport the_patch

But change mozilla-aurora to each branch. I'll get back to you when I've got it done.
(Reporter)

Comment 103

4 years ago
Aurora has the changes already. So just
hg up -C mozilla-beta
hg qimport the_patch
hg qpush

And then also qimport the patch from bug 1054189 that is not included in your patch and qpush it.
Do testruns, export the patch and add it here. 

Then for release and esr you're going to import just the final beta patch which has all the changes needed.
(Reporter)

Comment 104

4 years ago
Kyle, were you able to make the backport here? We have the merge today so we'd need this in beta before in order to get it to release as well.
(Assignee)

Comment 105

4 years ago
I'll have it up shortly.
(Assignee)

Comment 106

4 years ago
Created attachment 8478447 [details] [diff] [review]
backport of bug 883120 patch

I did the backport, as you outlined, but there's a few things I noticed. First of all, the file with henrik's fix lacked the proper prefixes, so instead of using his fix I simply redid the whole file. I'm slightly concerned there's others like this.

The other thing worth pointing out is how many files I worked on that simply don't exist any more. They cause error messages when I push the patch. Doesn't seem to affect test results, but figured I'd bring it up in case it proves important.

When you say "Then for release and esr you're going to import just the final beta patch which has all the changes needed." What exactly is it I need to do? Isn't the final beta patch the one I'm uploading here?
Attachment #8478447 - Flags: review?(andreea.matei)
Kyle, your problem here is that we had to move a lot of library files via bug 1036848 recently. So the files are not gone but on a new location. Sadly we had to do it, and weren't able to wait longer. :/ I know it's a pity for you know, but hopefully you find the time to update the patch. I think it should be enough to change the individual file locations in the patch for the new location.
(Assignee)

Comment 108

4 years ago
Ok, I'll try that.
(Reporter)

Comment 109

4 years ago
Kyle, so there's no point in reviewing the patch now right? There was the issue with libraries missing as Henrik pointed out. Hope it works easily if you change the location for the previous patch to apply and then move them back. Thanks!
(Reporter)

Updated

4 years ago
Attachment #8478447 - Flags: review?(andreea.matei)
Kyle, so just an example what needs to be done:

Current state in your patch:

diff -r 295fa1fe73a2 -r cbe9984acfd0 firefox/lib/downloads.js
--- a/firefox/lib/downloads.js	Wed Aug 20 09:46:42 2014 +0300
+++ b/firefox/lib/downloads.js	Tue Jul 22 23:15:29 2014 -0400

New state in your patch:

diff -r 295fa1fe73a2 -r cbe9984acfd0 lib/downloads.js
--- a/lib/downloads.js	Wed Aug 20 09:46:42 2014 +0300
+++ b/lib/downloads.js	Tue Jul 22 23:15:29 2014 -0400

That means do a search and replace for those modules, which have been moved from firefox/libs to libs/.
(Assignee)

Comment 111

4 years ago
That's completely understandable. I'm in the middle of moving to a new town, so I'll have this done by early next week.
Depends on: 1060599
I had to do another round of updates for a patch on bug 1060599. So please take those changes in consideration when you work on the release backport.
(Assignee)

Comment 113

4 years ago
I'll have that included when I upload it. Sorry about being quiet, the internet around here has been down for a while. I aim to get it uploaded on sunday.
(Assignee)

Comment 114

4 years ago
Well, now I've really gotten myself confused. I had two patches in my queue. bug883120, and bustage_fix. At first, I updated my repo, and tried to push my patch. I got all sorts of hunks failed. It said the patch failed. When I looked over them, it seems the changes were already applied. As an example,

-    this._pushInfo(name, message);
+    this._pushInfo(aName, aMessage);

Was part of the .rej file. When I looked at the file, it looked exactly like it should have:

this._pushInfo(aName, aMessage);

Then, I accidentally put in a "hg qpush" and it applied the bustage_fix. I tried to go back to my patch, so I put in a "hg qpop". However, now, I can apply my patch cleanly without any sorts of failures. I'm not sure how pushing a different patch and then popping it solved all my problems and made the patch not fail, so I figured I'd ask you guys what's going on. The patch should be the same as I uploaded before.



On a different note, Henrik, I took a look at the bug saying that you had moved files over, so I looked at the directories for all the files you moved. There were no instances of any directories pointing to any non-existent files. In the example you gave, downloads should be under /firefox/lib/.
Depends on: 1063584
(Reporter)

Comment 115

4 years ago
Kyle, it seems it's trickier as we have changed a lot of code in the repository meanwhile. If you don't mind, I'll take this backporting stuff further as it's more complicated. 
Please let me know whenever you'll want to try another bug, thanks!
(Assignee)

Comment 116

4 years ago
Alright, sorry about this. I've spent the past week trying different fixes, to no avail. I'm going to spend this weekend wiping everything and starting off clean.

Once I get that finished, I'll take a look into bug 1000832.
(Reporter)

Comment 117

4 years ago
Created attachment 8495292 [details] [diff] [review]
backport_release.patch

Thanks Kyle, but I worked a bit on it as it's getting harder to backport other bugs. I managed to move the libs back where they were before, apply the patch, solve some .rej and have this version.
There's no point in you to waste that much time again in making every change manually, thanks for your work! This is still assigned to you and you get all the credit for the refactoring, it was a big patch you created here :)

Tests (update failed as I already had the latest, but i was looking for typos/misplaced files):
http://mozmill-crowd.blargon7.com/#/functional/report/f11964362bbc85ebfbc5b36de7745472
http://mozmill-crowd.blargon7.com/#/endurance/report/f11964362bbc85ebfbc5b36de7747fd3
http://mozmill-crowd.blargon7.com/#/remote/report/f11964362bbc85ebfbc5b36de773f0c0
http://mozmill-crowd.blargon7.com/#/addons/report/f11964362bbc85ebfbc5b36de775096e
http://mozmill-crowd.blargon7.com/#/update/report/f11964362bbc85ebfbc5b36de774bb03
http://mozmill-crowd.blargon7.com/#/update/report/f11964362bbc85ebfbc5b36de774b0c3

I'll check esr tomorrow.
Attachment #8495292 - Flags: review?(andrei.eftimie)

Comment 118

4 years ago
Comment on attachment 8495292 [details] [diff] [review]
backport_release.patch

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

There's one conflict on mozilla-release in `firefox/lib/software-update.js`

And you missed to update a couple instances.

utils.js:
> @@ -289,19 +289,19 @@ function getDefaultHomepage() {
>    var prefValue = preferences.getPref("browser.startup.homepage", "",
>                                        true, Ci.nsIPrefLocalizedString);
>    return prefValue.data;
>  }
> 
>  /**
>   * Returns the value of an individual entity in a DTD file.
>   *
> - * @param [string] aUrls
> + * @param [string] urls
>   *        Array of DTD urls.
> - * @param {string} aEntityId
> + * @param {string} entityId


> @@ -121,17 +121,17 @@ var appInfo = {
>  };
> 
>  /**
>   * Assert if the current URL is identical to the target URL.
>   * With this function also redirects can be tested.
>   *
>   * @param {MozmillController} aController
>   *        MozMillController of the window to operate on
> - * @param {string} aTargetUrl
> + * @param {string} aTargetURL

> +++ b/lib/localization.js
> @@ -115,27 +115,27 @@ function checkDimensions(aChild) {
>    // check for horizontal overflow bigger than the tolerance,
>    // for elements higher than the tolerance
>    if (childBox.offsetHeight > tolerance &&
>        childBox.screenX + tolerance < parentBox.screenX) {
>      pixels = parentBox.x - childBox.x;
>      badRects.push([childBox.x, childBox.y, pixels,
>                     childBox.height]);
>      expect.fail('Node is cut off by ' + pixels +
> -                ' px at the left: ' + _reportNode(child) +
> +                ' px at the right: ' + _reportNode(aChild) +
>                  '. Parent node: ' + _reportNode(parent));
>    }
>    if (childBox.offsetHeight > tolerance &&
>        childBox.screenX + childBox.offsetWidth >
>        parentBox.screenX + parentBox.offsetWidth + tolerance) {
>      pixels = childBox.x + childBox.offsetWidth - parentBox.x - parentBox.offsetWidth;
>      badRects.push([parentBox.x + parentBox.offsetWidth, childBox.y,
>                     pixels, childBox.offsetHeight]);
>      expect.fail('Node is cut off by ' + pixels +
> -                ' px at the right: ' + _reportNode(aChild) +
> +                ' px at the right: ' + _reportNode(child) +
>                  '. Parent node: ' + _reportNode(parent));
>    }
> 
>    // check for vertical overflow bigger than the tolerance,
>    // for elements wider than the tolerance
>    // We don't want to test menupopup's, as they always report the full height
>    // of all items in the popup
>    if (aChild.nodeName != 'menupopup' && parent.nodeName != 'menupopup') {
> @@ -171,17 +171,17 @@ function checkDimensions(aChild) {
>   * @returns Filter status of the given node
>   * @type {array of array of int}
>   */
>  function filterAccessKeys(aNode) {
>    // Menus will need a separate filter set
>    var notAllowedLocalNames = ["menu", "menubar", "menupopup", "popupset"];
> 
>    if (!aNode.disabled && !aNode.collapsed && !aNode.hidden &&
> -      notAllowedLocalNames.indexOf(aNode.localName) == -1) {
> +      notAllowedLocalNames.indexOf(node.localName) == -1) {
>      // Code specific to the preferences panes to reject out not visible nodes
>      // in the panes.
>      if (aNode.parentNode && (aNode.parentNode.localName == "prefwindow" &&
>                               aNode.parentNode.currentPane.id != aNode.id) ||
>          ((aNode.parentNode.localName == "tabpanels" ||
>            aNode.parentNode.localName == "deck") &&
>            aNode.parentNode.selectedPanel.id != aNode.id)) {
>        return domUtils.DOMWalker.FILTER_REJECT;
Attachment #8495292 - Flags: review?(andrei.eftimie) → review-
(Reporter)

Comment 119

4 years ago
Created attachment 8496017 [details] [diff] [review]
backport_release.patch

Updated the issues.
Attachment #8495292 - Attachment is obsolete: true
Attachment #8496017 - Flags: review?(andrei.eftimie)

Comment 120

4 years ago
Comment on attachment 8496017 [details] [diff] [review]
backport_release.patch

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

This looks good, but landing of bug 1071590 minutes ago invalidated the changes to softwareUpdate tests and we have conflicts here.
Attachment #8496017 - Flags: review?(andrei.eftimie) → review-

Updated

4 years ago
Depends on: 1078137
status-firefox31: affected → ---

Comment 121

4 years ago
32 got all these changes with the merge in bug 1078137.

Only ESR31 remains.
status-firefox32: affected → fixed
(In reply to Andrei Eftimie from comment #121)
> 32 got all these changes with the merge in bug 1078137.

Not true. We merged beta into release, so release is 33.0 now. So we can't fix it for Firefox 32.0.x.
status-firefox32: fixed → wontfix

Comment 123

4 years ago
(In reply to Henrik Skupin (:whimboo) from comment #122)
> (In reply to Andrei Eftimie from comment #121)
> > 32 got all these changes with the merge in bug 1078137.
> 
> Not true. We merged beta into release, so release is 33.0 now. So we can't
> fix it for Firefox 32.0.x.

Right.
This doesn't meet ESR landing criteria unless it's an a=test-only, marking wontfix to keep it out of our triage queue.
status-firefox-esr31: affected → wontfix
(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #124)
> This doesn't meet ESR landing criteria unless it's an a=test-only, marking
> wontfix to keep it out of our triage queue.

Lukas, as mentioned a couple of times you might want to adjust your query. This bug is not about Firefox but in a totally different product. Please exclude Mozmill Tests in your query. Thanks.
status-firefox-esr31: wontfix → affected
Flags: needinfo?(lsblakk)
Thanks Henrik, I will go adjust those queries.
Flags: needinfo?(lsblakk)
status-firefox-esr31: affected → wontfix
(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #126)
> Thanks Henrik, I will go adjust those queries.

I hope that this will happen soon so that we do not have to revert those changes, as done again by dveditz last night, each time. Thanks.
status-firefox-esr31: wontfix → affected
Sorry about that. We're excluding the "Testing" product, didn't notice "Mozilla QA" in there too.
What about the tracking-firefox-esr31 flag? release-drivers still own that right? If so we can minus bugs to get them off the query instead of messing with the status- flag.
Flags: needinfo?(hskupin)
(In reply to Daniel Veditz [:dveditz] from comment #129)
> What about the tracking-firefox-esr31 flag? release-drivers still own that
> right? If so we can minus bugs to get them off the query instead of messing
> with the status- flag.

Offtopic but lets finish it off. So we don't use that flag right now, but it might be best to also remove it from your query in case of Testing and Mozilla QA. It might help us to use in the future if must-have fixes for specific releases are flagged.
Flags: needinfo?(hskupin)

Comment 131

4 years ago
Setting a flag to not forget, this bug only needs backporting to ESR31 to close it.
Flags: needinfo?(andreea.matei)
Lets wontfix the backport request for esr31. This is not necessary anymore. All other branches are fixed. Thanks a lot for working on it Kyle!
Mentor: matei.andreea89
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago3 years ago
status-firefox-esr31: affected → wontfix
Flags: needinfo?(matei.andreea89)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.