Status

()

Core
DOM
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: roc, Assigned: poiru)

Tracking

(Depends on: 3 bugs, Blocks: 1 bug, {leave-open})

Trunk
leave-open
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(20 attachments, 3 obsolete attachments)

45.97 KB, patch
Ms2ger
: review+
roc
: checkin+
Details | Diff | Splinter Review
38.44 KB, patch
Ms2ger
: review+
roc
: checkin+
Details | Diff | Splinter Review
47.15 KB, patch
Ms2ger
: review+
roc
: checkin+
Details | Diff | Splinter Review
110.96 KB, patch
Ms2ger
: review+
roc
: checkin+
Details | Diff | Splinter Review
815 bytes, patch
armenzg
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
1.25 KB, patch
smaug
: review+
roc
: checkin+
Details | Diff | Splinter Review
1.35 KB, patch
smaug
: review+
roc
: checkin+
Details | Diff | Splinter Review
551 bytes, patch
Callek
: review+
Details | Diff | Splinter Review
1.22 KB, patch
aki
: review+
Details | Diff | Splinter Review
27.77 KB, patch
Details | Diff | Splinter Review
4.46 KB, patch
karlt
: review+
Details | Diff | Splinter Review
99.69 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
304.77 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
142.86 KB, patch
janv
: review+
Details | Diff | Splinter Review
39.36 KB, patch
peterv
: review+
Details | Diff | Splinter Review
371.46 KB, patch
Details | Diff | Splinter Review
48.96 KB, patch
peterv
: review+
Details | Diff | Splinter Review
245.69 KB, patch
Details | Diff | Splinter Review
110.07 KB, patch
peterv
: review+
Details | Diff | Splinter Review
473.77 KB, patch
Details | Diff | Splinter Review
Flattening away unnecessary directories means slightly faster builds and slightly more convenient development (content/html/HTMLMediaElement.cpp FTW).

Basically I propose eliminating all "public" and "src" directories, moving their contents up to the parent directory. Also, eliminate all "content" and "document" directories, moving their contents up to the parent directory. This will require merging of some test directories.

content/base/public/*
content/base/src/*
  --> content/base

content/html/content/*
content/html/content/public/*
content/html/content/src/*
content/html/document/*
content/html/document/public/*
content/html/document/src/*
  --> content/html

content/xml/content/*
content/xml/content/src/*
content/xml/document/*
content/xml/document/public/*
content/xml/document/src/*
  --> content/xml

content/events/public/*
content/events/src/*
  -> content/events

content/xul/content/*
content/xul/content/public/*
content/xul/content/src/*
content/xul/document/*
content/xul/document/public/*
content/xul/document/src/*
  --> content/xul

content/xul/templates/public/*
content/xul/templates/src/*
  --> content/xul/templates

content/mathml/content/*
content/mathml/content/src/*
  --> content/mathml

content/xslt/public/*
content/xslt/src/*
  --> content/xslt

content/canvas/public/*
content/canvas/src/*
  --> content/canvas

content/svg/content/*
content/svg/content/src/*
content/svg/document/*
content/svg/document/src/*
  --> content/svg

content/xbl/src/*
  --> content/xbl
I do not like having the source files next to the test directories, because it makes mxr less useful and local grep a little more annoying to use. I not infrequently find myself searching for a string but wanting to filter the results both to those that contain a second string, and those that are in source files in a certain directory. When the tests are under the source directory I can't get mxr to filter out results in the test files in this scenario. Local grep'ing has a similar problem, but it's just more annoying to get what you want rather than being impossible as it is with mxr.

I'd rather have side-by-side 'src' and 'test' directories personally.
MXR's file path takes a regexp. So you can limit to, e.g., content/media/[^/]*$. Local grep is even easier, it's just "grep foobar content/media/*".

Also, most code in mozilla-central doesn't use 'src', so there's a consistency argument here too.
As long as this is done in a way that preserves blame I'm all for it.
These would all be history-preserving hg moves for source files. We'd have to merge some moz.build and test manifest files which means for each set of merged files, we'd do an hg move of the biggest one to keep its history and lose history on the others.
We definitely should drop the src and public dirs; not sure about dropping content/document.

But if we're doing these moves, we might as well move everything to dom.
Yeah, we could do that too.

I had been thinking that it's sort-of-nice to have content/ containing everything element and document related while dom/ contains all the other DOM APIs, but that's not really true; content/base contains all kinds of random stuff, and content/canvas, content/events and content/media don't belong in content/ under that definition. So I think moving everything to dom/ makes sense.
Moving them both into dom/ would be nice.  I still have no general mental model for figuring out what lives where, so I just end up memorizing specific cases.
So roc do you have a proposal based on comment 6?
Sure. The proposal is to do everything in comment #0 and also move everything under content/ to dom/.
Johnny, do you agree that we should do this?
Flags: needinfo?(jst)
This change is going to break my fingers, but I'm looking forward to it!

I was going to ask about keeping elements in their own subdirectories (e.g. dom/html/elements/HTMLElement.cpp), but then I realized that the parent directory would basically be empty, so there's no obvious reason to do that.
Yeah, I'm in favor of this as well, what I haven't made up my mind about is what to do about tests. roc, is your plan to leave tests in a tests directory much like they are today, just merge some of those tests directories together as we eliminate our unnecessarily deep structure for our source/headers here?
Flags: needinfo?(jst)
Yes.
Note that moving everything to dom/ means merging content/base with dom/base and content/media with dom/media. That seems OK to me.
Created attachment 8348451 [details] [diff] [review]
Part 1: Canvas

https://tbpl.mozilla.org/?tree=Try&rev=5746442456c9
Attachment #8348451 - Flags: review?(Ms2ger)
Comment on attachment 8348451 [details] [diff] [review]
Part 1: Canvas

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

Note that the path to content/canvas/test/webgl is hardcoded for android mochitest-gl; e.g. <http://hg.mozilla.org/build/mozharness/file/7a05bfd62005/configs/android/android_panda_releng.py#l113>.

::: dom/canvas/moz.build
@@ +26,1 @@
>  EXPORTS.mozilla.dom += [

Nit: newline between those

::: dom/canvas/test/android.json
@@ +1,1 @@
>  {

I need to find someone who knows what this file is for.

::: dom/canvas/test/test_canvas.html
@@ +80,5 @@
>    return enabled;
>  }
>  
>  </script>
> +<!-- Includes all the tests in the dom/canvas/tests except for test_bug397524.html -->

/test, not /tests, and please "most of the tests"

::: layout/build/moz.build
@@ +55,5 @@
>      '/docshell/base',
>      '/dom/audiochannel',
>      '/dom/base',
>      '/dom/camera',
> +    '/dom/canvas/src',

I don't think there's a dom/canvas/src. If it isn't necessary, remove it?
jmaher, how do we get the mochitest-gl path updated?
Flags: needinfo?(jmaher)
(In reply to :Ms2ger from comment #16)
> I don't think there's a dom/canvas/src. If it isn't necessary, remove it?

It must not be necessary, so I removed it. Fixed everything else. Thanks!
For mochitest-gl, we run that as a separate chunk on tbpl. You would need to modify the source and the mozharness script. I am not sure how to do that at the same time- if there isn't a way could we duplicate the tests, modify mozharness, then remove the old tests?

:aki, how can we update the mozharness script at the same time we update m-c?
Flags: needinfo?(jmaher) → needinfo?(aki)

Comment 20

3 years ago
(In reply to Joel Maher (:jmaher) from comment #19)
> For mochitest-gl, we run that as a separate chunk on tbpl. You would need to
> modify the source and the mozharness script. I am not sure how to do that at
> the same time- if there isn't a way could we duplicate the tests, modify
> mozharness, then remove the old tests?
> 
> :aki, how can we update the mozharness script at the same time we update m-c?

I'm not familiar with the mochitest-gl test, but we can either do as you suggest, or we can have the patch ready in mozharness, and land that after the m-c patch lands.  However, this will be global, so if mochitest-gl runs on other branches, those paths will be wrong until the patch lands on all branches we run mochitest-gl on.

If we moved the logic for what to run into the tree, and then had mozharness call a script or read config (and if that path is the same everywhere) then that would help avoid some of this.
Flags: needinfo?(aki)
when you say other branches, do you mean aurora/beta/release, or just trunk based branches?

Comment 22

3 years ago
all of the above, plus esr*s and b2g*s, if mochitest-gl is running there.
sounds like we will need to come up with another approach here.

Can we roll out a new test 'mochitest-gl2' which is a different directory?

Another option is to change it to point to a non-existent directory (so we have two of them) and the missing directory would finish quickly and the full one would test as usual- then we could move the directories and let it ride the trains.
(In reply to Joel Maher (:jmaher) from comment #23)
> Another option is to change it to point to a non-existent directory (so we
> have two of them) and the missing directory would finish quickly and the
> full one would test as usual- then we could move the directories and let it
> ride the trains.

So basically mozharness would have two directories listed, and only test the directory that exists? If we can do that, that sounds ideal to me.
ok, I did a test with adding a file gl.json to the mochitest directory:
{
 "runtests": {"content/canvas/test/webgl": "original location",
              "content/webgl": "new location"},
 "excludetests": {}
}


and then running with "--run-only-tests gl.json" instead of "--test-path=content/canvas/test/webgl"

This worked fine on my linux desktop, sounds like a solution we could employ to make this work although it is hacky.  Aki, this is a starting point, possibly you know a better solution?
Flags: needinfo?(aki)

Comment 26

3 years ago
I think the 2 directory solution sounds great as well.

I'm not sure if you want to do some sort of check for the existence gl.json and fall back to the old --test-path if it doesn't exist, or push gl.json to all trees that are running mochitest-gl and then switch over to using that file for all mochitest-gl runs, but I think it works til these changes have populated through all trees.

(If mochitest-gl is only on the standard release trains, then that'll be in a few months' time; if it's on ESR24 we may need to support gl.json quite a bit longer.)
Flags: needinfo?(aki)
If I can get a new directory name for the webgl tests, I can create and land the file.  Once that is in, we can make the mozharness changes.
Ms2ger pointed out that we could simplify this:
1) create gl.json in the tree- pointing to the single directory
2) start letting it merge
3) get mozharness to support it
4) when we flatten the directory structure, we can adjust gl.json to be the new directory name, it will ride the merges and trains together

does this sound logical?
Comment on attachment 8348451 [details] [diff] [review]
Part 1: Canvas

Cancelling review until we get the m-gl stuff done.
Attachment #8348451 - Flags: review?(Ms2ger)
(In reply to Joel Maher (:jmaher) from comment #28)
> Ms2ger pointed out that we could simplify this:
> 1) create gl.json in the tree- pointing to the single directory
> 2) start letting it merge
> 3) get mozharness to support it
> 4) when we flatten the directory structure, we can adjust gl.json to be the
> new directory name, it will ride the merges and trains together
> 
> does this sound logical?

Does this mean between step 2 and step 3 we have to wait for all active trees, including ESR, to get the merged file? i.e. October 2014 or so?
Created attachment 8351624 [details] [diff] [review]
Part 2: Move content/xslt to dom/xslt and flatten away 'public' and 'src'

https://tbpl.mozilla.org/?tree=Try&rev=305a2ebb2913
Attachment #8351624 - Flags: review?(Ms2ger)
Created attachment 8351625 [details] [diff] [review]
Part 2.1: Remove dom/xslt/tests/buster/moz.build, which is empty
Attachment #8351625 - Flags: review?(Ms2ger)
Created attachment 8351689 [details] [diff] [review]
Part 3: Move content/smil to dom/smil
Attachment #8351689 - Flags: review?(Ms2ger)
Comment on attachment 8351625 [details] [diff] [review]
Part 2.1: Remove dom/xslt/tests/buster/moz.build, which is empty

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

$ cat content/xslt/tests/buster/moz.build
# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
# vim: set filetype=python:
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.

JAR_MANIFESTS += ['jar.mn']
Attachment #8351625 - Flags: review?(Ms2ger) → review-

Updated

3 years ago
Attachment #8351689 - Flags: review?(Ms2ger) → review+

Updated

3 years ago
Attachment #8351624 - Flags: review?(Ms2ger) → review+
Please do ask a peer to sr before landing, though.
(In reply to :Ms2ger from comment #34)
> JAR_MANIFESTS += ['jar.mn']

I don't know what I was thinking...
(In reply to :Ms2ger from comment #35)
> Please do ask a peer to sr before landing, though.

I got verbal approval from Johnny, Kyle and Blake above, so I don't think a formal sr is necessary. But if you say so...
Attachment #8351624 - Flags: superreview?(khuey)
Attachment #8351689 - Flags: superreview?(khuey)
Attachment #8351624 - Flags: superreview?(khuey) → superreview+
Attachment #8351689 - Flags: superreview?(khuey) → superreview+
https://hg.mozilla.org/integration/mozilla-inbound/rev/67ff9392766b
https://hg.mozilla.org/integration/mozilla-inbound/rev/215f5bc6284d
Whiteboard: [leave open]
Attachment #8351624 - Flags: checkin+
Attachment #8351689 - Flags: checkin+
Attachment #8351625 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/67ff9392766b
https://hg.mozilla.org/mozilla-central/rev/215f5bc6284d
Created attachment 8356747 [details] [diff] [review]
Part 4: XBL
Attachment #8356747 - Flags: review?(Ms2ger)
Created attachment 8356748 [details] [diff] [review]
Part 5: Events
Attachment #8356748 - Flags: review?(Ms2ger)
Comment on attachment 8356747 [details] [diff] [review]
Part 4: XBL

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

r=me

::: accessible/src/base/moz.build
@@ +58,5 @@
>          'Logging.cpp',
>      ]
>  
>  LOCAL_INCLUDES += [
> +    '../../../dom/xbl',

Nit: make it '/dom/xbl' while you're here

::: content/svg/content/src/moz.build
@@ +251,5 @@
>  LOCAL_INCLUDES += [
>      '/content/base/src',
>      '/content/events/src',
>      '/content/html/content/src',
>      '/content/xbl/src',

Remove

::: layout/base/moz.build
@@ +115,5 @@
>      '../../content/base/src',
>      '../../content/events/src',
>      '../../content/html/content/src',
>      '../../content/svg/content/src',
>      '../../content/xbl/src',

Remove
Attachment #8356747 - Flags: review?(Ms2ger) → review+
Comment on attachment 8356748 [details] [diff] [review]
Part 5: Events

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

r=me

However, after this patch, we've got dom/events and dom/src/events; to avoid confusion, could you please move dom/src/events/nsJSEventListener.{h,cpp} (yes, that's all that lives there) and dom/base/nsIJSEventListener.h to dom/events as well?

::: accessible/src/windows/msaa/moz.build
@@ +45,5 @@
>      ]
>  
>  LOCAL_INCLUDES += [
>      '../../../../content/base/src',
> +    '../../../../dom/events',

Nit: '/dom/events'

::: layout/xul/moz.build
@@ +87,5 @@
>  
>  FINAL_LIBRARY = 'gklayout'
>  LOCAL_INCLUDES += [
>      '../../content/base/src',
> +    '../../dom/events',

Nit: /dom/events

::: view/src/moz.build
@@ +16,5 @@
>  
>  FINAL_LIBRARY = 'gklayout'
>  
>  LOCAL_INCLUDES += [
> +    '../../dom/events/',

Nit: /dom/events
Attachment #8356748 - Flags: review?(Ms2ger) → review+
Sure(In reply to :Ms2ger from comment #43)
> Comment on attachment 8356748 [details] [diff] [review]
> Part 5: Events
> 
> Review of attachment 8356748 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me
> 
> However, after this patch, we've got dom/events and dom/src/events; to avoid
> confusion, could you please move dom/src/events/nsJSEventListener.{h,cpp}
> (yes, that's all that lives there) and dom/base/nsIJSEventListener.h to
> dom/events as well?

Sure
jmaher, see comment #30
Flags: needinfo?(jmaher)
roc, sorry I missed this.  We would have to wait for it to arrive on all trees that we run mochitest-gl on android.  This is not ESR, nor some of the b2g branches.  We would need this to make it to mozilla-release though.  As this is a test only change, it could be landed on beta/aurora/central and be resolved fairly quickly
Flags: needinfo?(jmaher)
Created attachment 8357936 [details] [diff] [review]
add gl.json to use for mochitest-gl (1.0)
Attachment #8357936 - Flags: review?(armenzg)
Attachment #8357936 - Flags: review?(armenzg) → review+
Thanks!!
Created attachment 8358057 [details] [diff] [review]
add gl.json to use for mochitest-gl (1.1)

forgot to copy this to the mochitest directory in tests packaging.
Attachment #8357936 - Attachment is obsolete: true
Attachment #8358057 - Flags: review?(armenzg)
Attachment #8358057 - Flags: review?(armenzg) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/243259fda9ab
Attachment #8356747 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/243259fda9ab
landed the gl changes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f86bd747899

we really should get these landed in other branches as well (aurora, beta, possibly release).
Comment on attachment 8358057 [details] [diff] [review]
add gl.json to use for mochitest-gl (1.1)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: none
Testing completed (on m-c, etc.): none
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

This is a test-only change. Getting on branches will help us fix this bug sooner.
Attachment #8358057 - Flags: approval-mozilla-beta?
Attachment #8358057 - Flags: approval-mozilla-aurora?
Comment on attachment 8358057 [details] [diff] [review]
add gl.json to use for mochitest-gl (1.1)

NPOTB changes do not require approval.
Attachment #8358057 - Flags: approval-mozilla-beta?
Attachment #8358057 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/5f86bd747899
The events patch makes test_object_plugin_nav.html fail on Mac. https://hg.mozilla.org/try/rev/7cf5e598d868
Created attachment 8359557 [details] [diff] [review]
Part 5.1: Enable test plugin for test_object_plugin_nav.html
Attachment #8359557 - Flags: review?(bugs)
Created attachment 8359560 [details] [diff] [review]
Part 5.2: Set accessibility.focusmodel in test_object_plugin_nav.html so we can tab to links even on Mac

I have no idea how this test was passing on Mac. Perhaps some test was setting accessibility.focusmodel and failing to clean up properly. Maybe the same goes for enabling the test plugin. Anyway with these patches, the test passes when run alone on the office Mac.
Attachment #8359560 - Flags: review?(bugs)

Updated

3 years ago
Attachment #8359557 - Flags: review?(bugs) → review+

Updated

3 years ago
Attachment #8359560 - Flags: review?(bugs) → review+
Comment on attachment 8358057 [details] [diff] [review]
add gl.json to use for mochitest-gl (1.1)

Checkin needed for all branches that run mochitest-gl.
Attachment #8358057 - Flags: checkin?
https://hg.mozilla.org/integration/mozilla-inbound/rev/35dddd51e275
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9cfed12c18d
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad1334d4a447
Attachment #8356748 - Flags: checkin+
Attachment #8359557 - Flags: checkin+
Attachment #8359560 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/35dddd51e275
https://hg.mozilla.org/mozilla-central/rev/d9cfed12c18d
https://hg.mozilla.org/mozilla-central/rev/ad1334d4a447
Comment on attachment 8358057 [details] [diff] [review]
add gl.json to use for mochitest-gl (1.1)

https://hg.mozilla.org/releases/mozilla-aurora/rev/a0484dca0920
https://hg.mozilla.org/releases/mozilla-beta/rev/fb53f8276dff
https://hg.mozilla.org/releases/mozilla-release/rev/39dc39aadc65

And a follow-up fix to inbound to fix the messed up indentation this added to Makefile.in.

https://hg.mozilla.org/integration/mozilla-inbound/rev/15999356c665
Attachment #8358057 - Flags: checkin? → checkin+
Created attachment 8360476 [details] [diff] [review]
adjust buildbot builders to use gl.json instead of test-path (1.0)
Attachment #8360476 - Flags: review?(bugspam.Callek)
Comment on attachment 8360476 [details] [diff] [review]
adjust buildbot builders to use gl.json instead of test-path (1.0)

untested, but looks sane
Attachment #8360476 - Flags: review?(bugspam.Callek) → review+
landed the change on buildbot-configs:
https://hg.mozilla.org/build/buildbot-configs/rev/816ed778c4de

we will need to wait for the next buildbot-config to take place, then this will be live.

Once this is live and verified to be solid- we can change gl.json on inbound as needed.
something is in production
https://hg.mozilla.org/mozilla-central/rev/15999356c665
Summary: Flatten content subdirectories → Move content/ to dom/
Can Part 1 land now, with the additional change to gl.json?
https://tbpl.mozilla.org/php/getParsedLog.php?id=33439212&tree=Mozilla-Inbound seems to still use the path directly.
Flags: needinfo?(jmaher)
Created attachment 8364286 [details] [diff] [review]
support mozharness mochitest-gl with manifest (1.0)

It appears that we did make some of our tests run on mozharness.  My error for the mistake here.  This patch will change the mozharness configs.
Attachment #8364286 - Flags: review?(aki)
Flags: needinfo?(jmaher)

Comment 71

3 years ago
Comment on attachment 8364286 [details] [diff] [review]
support mozharness mochitest-gl with manifest (1.0)

Does this file exist on all branches where we run this test?
If not, it will burn.
Attachment #8364286 - Flags: review?(aki) → review+
yes, this had been landed on all branches.
landed on mozharness branch:
https://hg.mozilla.org/build/mozharness/rev/108a5bcff97c

I assume this will get picked up when we update mozharness to production in the next day or two.
a mozharness patch is in production! :)
(Assignee)

Comment 75

3 years ago
Created attachment 8402393 [details] [diff] [review]
Part 6: Move content/xml/ to dom/ and flatten subdirectories

roc, I took the liberty of assigning this bug to myself. If you're still working on this, feel free to change it back.
Assignee: roc → birunthan
Status: NEW → ASSIGNED
Attachment #8402393 - Flags: superreview?(jst)
(Assignee)

Comment 76

3 years ago
Created attachment 8402394 [details] [diff] [review]
Part 7: Move content/mathml/ to dom/ and flatten subdirectories
Attachment #8402394 - Flags: superreview?(karlt)
(Assignee)

Comment 77

3 years ago
Created attachment 8402395 [details] [diff] [review]
Part 8: Move content/svg/ to dom/ and flatten subdirectories

Try push: https://tbpl.mozilla.org/?tree=Try&rev=f9f900dbdae5
Attachment #8402395 - Flags: superreview?(jwatt)
Attachment #8402394 - Flags: superreview?(karlt) → review+
Comment on attachment 8402393 [details] [diff] [review]
Part 6: Move content/xml/ to dom/ and flatten subdirectories

Looks good!
Attachment #8402393 - Flags: superreview?(jst) → superreview+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [leave open] → [leave open][checkin parts 6 & 7]
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2350812b7f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a0290190c1b
Flags: in-testsuite-
Keywords: checkin-needed
Whiteboard: [leave open][checkin parts 6 & 7] → [leave open]
backed out this changes since they might have caused Bug 963244
(Assignee)

Updated

3 years ago
Depends on: 963244

Updated

3 years ago
Attachment #8402395 - Flags: superreview?(jwatt) → review+
(Assignee)

Comment 81

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b673ac36f66
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b6b04006c1b
https://hg.mozilla.org/integration/mozilla-inbound/rev/85228ee8ce8b
No longer depends on: 963244
Whiteboard: [leave open]
(Assignee)

Updated

3 years ago
Keywords: leave-open
Backed these out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6f0b312e09aa for apparently introducing a failure in b2g mochitest-6:

https://tbpl.mozilla.org/php/getParsedLog.php?id=39516635&tree=Mozilla-Inbound
Flags: needinfo?(birunthan)
(Assignee)

Comment 83

3 years ago
(In reply to Wes Kocher (:KWierso) from comment #82)
> Backed these out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6f0b312e09aa for
> apparently introducing a failure in b2g mochitest-6:
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=39516635&tree=Mozilla-
> Inbound

Apologies, it seemed to work on try: https://tbpl.mozilla.org/?tree=Try&rev=e5fcee3e8700 (guess I should've tested debug as well). This was most likely caused by test reordering, but in an ideal world that wouldn't break anything. I'll try again if and when this test is disabled on B2G ;)
Depends on: 958689
Flags: needinfo?(birunthan)
(Assignee)

Comment 84

3 years ago
Created attachment 8462575 [details] [diff] [review]
Part 1: Move content/canvas/ to dom/ and flatten subdirectories
Attachment #8348451 - Attachment is obsolete: true
Attachment #8462575 - Flags: review?(roc)
Comment on attachment 8462575 [details] [diff] [review]
Part 1: Move content/canvas/ to dom/ and flatten subdirectories

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

This patch looks odd. Shouldn't there be some file moves?

I had a previous version of this patch with file moves, attached to this bug, but every time I looked at landing it, there were test failures, probably due to moved tests :-(.

I'm not a content/DOM peer so I can't review this, sorry.
Attachment #8462575 - Flags: review?(roc)
(Assignee)

Comment 86

3 years ago
Comment on attachment 8462575 [details] [diff] [review]
Part 1: Move content/canvas/ to dom/ and flatten subdirectories

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #85)
> Comment on attachment 8462575 [details] [diff] [review]
> Part 1: Move content/canvas/ to dom/ and flatten subdirectories
> 
> Review of attachment 8462575 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch looks odd. Shouldn't there be some file moves?

There are file moves, but Bugzilla's UI doesn't show them. Try viewing the raw patch instead.

> I had a previous version of this patch with file moves, attached to this
> bug, but every time I looked at landing it, there were test failures,
> probably due to moved tests :-(.

Yep, I have been bitten by this several times, too. The try run looks good, though: https://tbpl.mozilla.org/?tree=Try&rev=4c94f4e7d3c9 (ignore the crashtest oranges)

I will push to try once more before attempting to land on inbound.
Attachment #8462575 - Flags: review?(bzbarsky)
Comment on attachment 8462575 [details] [diff] [review]
Part 1: Move content/canvas/ to dom/ and flatten subdirectories

I'm swamped with reviews.  Let's try jst, since everyone else is on PTO.

Alternately, I can look at this when I get back Aug 19.
Attachment #8462575 - Flags: review?(bzbarsky) → review?(jst)
Comment on attachment 8462575 [details] [diff] [review]
Part 1: Move content/canvas/ to dom/ and flatten subdirectories

Or better yet, Ehsan.
Attachment #8462575 - Flags: review?(jst) → review?(ehsan)
Attachment #8462575 - Flags: review?(ehsan) → review+
(Assignee)

Comment 89

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c197ac16fbc
https://hg.mozilla.org/integration/mozilla-inbound/rev/a947d30dc810
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf3d1eb30dcf

Try push: https://tbpl.mozilla.org/?tree=Try&rev=1cf2531583c2
(In reply to Birunthan Mohanathas [:poiru] from comment #89)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1c197ac16fbc
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a947d30dc810
> https://hg.mozilla.org/integration/mozilla-inbound/rev/bf3d1eb30dcf
> 
> Try push: https://tbpl.mozilla.org/?tree=Try&rev=1cf2531583c2

I had to clobber to get some failures to go away on this push, so here's a followup CLOBBER touch so it doesn't break something as it gets merged around: https://hg.mozilla.org/integration/mozilla-inbound/rev/af1e635f0846

The failure was https://tbpl.mozilla.org/php/getParsedLog.php?id=44622359&tree=Mozilla-Inbound

Updated

3 years ago
Blocks: 1044293
Really every future patch in this bug should land with a CLOBBER change!
https://hg.mozilla.org/mozilla-central/rev/1c197ac16fbc
https://hg.mozilla.org/mozilla-central/rev/a947d30dc810
https://hg.mozilla.org/mozilla-central/rev/bf3d1eb30dcf
https://hg.mozilla.org/mozilla-central/rev/af1e635f0846
(Assignee)

Comment 93

3 years ago
Created attachment 8466710 [details] [diff] [review]
Part 9: Move content/xul/ to dom/ and flatten subdirectories
Attachment #8466710 - Flags: review?(Jan.Varga)
(Assignee)

Updated

3 years ago
Depends on: 1048349

Updated

3 years ago
Attachment #8466710 - Flags: review?(Jan.Varga) → review+
(Assignee)

Comment 94

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c4e45496cac
https://hg.mozilla.org/integration/mozilla-inbound/rev/69551e5b1a5d

Try push: https://tbpl.mozilla.org/?tree=Try&rev=89103ce3ebb6
Followup touching clobber, since this will likely need one:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d99f8aaeb32b
(try builds are !clobber)
(In reply to Birunthan Mohanathas [:poiru] from comment #94)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4c4e45496cac
> https://hg.mozilla.org/integration/mozilla-inbound/rev/69551e5b1a5d
> 
> Try push: https://tbpl.mozilla.org/?tree=Try&rev=89103ce3ebb6

Backed out for mochitest-2 crashes on debug Windows:
https://tbpl.mozilla.org/php/getParsedLog.php?id=45334834&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=45335357&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=45335599&tree=Mozilla-Inbound

Note these crashes were present in the try run too.

When this relands please also remember to touch the CLOBBER file - thanks! :-)

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8b65ed5b3507
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a7180856b416
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8d97586ffcfc
Also this leak on b2g:
https://tbpl.mozilla.org/php/getParsedLog.php?id=45337296&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=45335295&tree=Mozilla-Inbound
(Assignee)

Updated

3 years ago
Depends on: 1084979
(Assignee)

Updated

3 years ago
Depends on: 1085208
(Assignee)

Comment 98

3 years ago
Created attachment 8507938 [details] [diff] [review]
Part 10: Move content/media/ to dom/

For easier reviews, I split these patches in two. This part contains the important stuff (i.e. changes to *.{ini,list,build,gyp,in} files). The next part consists of trivial moves and e.g. fixes to paths in tests.

Note that this patch does not merge content/media/test with the existing dom/media/tests directory. I will file a follow-up for that.
Attachment #8507938 - Flags: review?(peterv)
(Assignee)

Comment 99

3 years ago
Created attachment 8507940 [details] [diff] [review]
Remaining changes for moving content/media/ to dom/
(Assignee)

Comment 100

3 years ago
Created attachment 8507941 [details] [diff] [review]
Part 11: Move content/html/ to dom/ and flatten subdirectories
Attachment #8507941 - Flags: review?(peterv)
(Assignee)

Comment 101

3 years ago
Created attachment 8507943 [details] [diff] [review]
Remaining changes for moving content/html/ to dom/
(Assignee)

Comment 102

3 years ago
Created attachment 8507944 [details] [diff] [review]
Part 12: Move content/base/ to dom/ and flatten subdirectories
Attachment #8507944 - Flags: review?(peterv)
(Assignee)

Comment 103

3 years ago
Created attachment 8507946 [details] [diff] [review]
Remaining changes for moving content/base/ to dom/
Comment on attachment 8507938 [details] [diff] [review]
Part 10: Move content/media/ to dom/

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

::: testing/web-platform/tests/webaudio/the-audio-api/the-mediaelementaudiosourcenode-interface/mediaElementAudioSourceToScriptProcessorTest.html
@@ +7,5 @@
>  array, and after the playback has stopped, the contents are compared
>  to those of a loaded AudioBuffer with the same source.
>  
>  Somewhat similiar to a test from Mozilla:
> +(http://mxr.mozilla.org/mozilla-central/dom/dom/media/webaudio/test/test_mediaElementAudioSourceNode.html?force=1)

This URL looks wrong, I think it needs mozilla-central/source/dom
Attachment #8507938 - Flags: review?(peterv) → review+
Comment on attachment 8507941 [details] [diff] [review]
Part 11: Move content/html/ to dom/ and flatten subdirectories

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

::: content/html/document/test/browser.ini
@@ -1,5 @@
> -[DEFAULT]
> -support-files = bug592641_img.jpg
> -
> -[browser_bug592641.js]
> -[browser_bug1081537.js]

What happened to this test?

::: dom/html/crashtests/crashtests.list
@@ +1,1 @@
> +load 1032654.html

Not sure that it's helpful to move this up.

::: dom/html/test/mochitest.ini
@@ +521,5 @@
>  skip-if = (toolkit == 'gonk' && debug) #debug-only failure
>  [test_fragment_form_pointer.html]
> +
> +
> +

Why all the whitespace?
Attachment #8507941 - Flags: review?(peterv) → review+
Comment on attachment 8507944 [details] [diff] [review]
Part 12: Move content/base/ to dom/ and flatten subdirectories

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

r+ though I'd really like to make sure that we're still running the tests given the removal of those *_MANIFESTS variables.

::: docshell/base/moz.build
@@ +72,5 @@
>  
>  FINAL_LIBRARY = 'xul'
>  LOCAL_INCLUDES += [
>      '../shistory/src',
> +    '/dom/base',

Remove this line.

::: dom/base/crashtests/crashtests.list
@@ +44,5 @@
>  load 898906.html
>  load 852381.html
>  load 973401.html
>  pref(dom.webcomponents.enabled,true) load 1029710.html
> +load 43040-1.html

Keep this file sorted.

::: dom/base/moz.build
@@ +342,5 @@
>      'nsPluginArray.cpp',
>  ]
>  
> +SOURCES += [
> +    'nsImageLoadingContent.cpp',

Add a comment here, mentioning that it's here because something redefines LoadImage (probably windows.h). See bug 949435 comment 87.

@@ +367,5 @@
> +        'nsDOMDataChannel.cpp',
> +    ]
> +    LOCAL_INCLUDES += [
> +        '/netwerk/sctp/datachannel',
> +    ]

Hmm, in the dom/media patch you spit these conditional blocks up and put them close to where the lists are defined, not in one block. Can you be consistent?

@@ -189,5 @@
>  if CONFIG['MOZ_BUILD_APP'] in ['browser', 'mobile/android', 'xulrunner']:
>      DEFINES['HAVE_SIDEBAR'] = True
>  
> -MOCHITEST_MANIFESTS += ['test/mochitest.ini']
> -MOCHITEST_CHROME_MANIFESTS += ['test/chrome.ini']

I don't understand this removal, why is this ok?

::: dom/browser-element/moz.build
@@ +37,3 @@
>      '/dom/',
>      '/dom/base',
> +    '/dom/base',

Remove this line.

::: dom/devicestorage/moz.build
@@ +30,5 @@
>  include('/ipc/chromium/chromium-config.mozbuild')
>  
>  FINAL_LIBRARY = 'xul'
>  LOCAL_INCLUDES += [
> +    '/dom/base',

Remove this line.

::: dom/geolocation/moz.build
@@ +19,5 @@
>  include('/ipc/chromium/chromium-config.mozbuild')
>  
>  FINAL_LIBRARY = 'xul'
>  LOCAL_INCLUDES += [
> +    '/dom/base',

Remove this line.

::: dom/indexedDB/moz.build
@@ +94,3 @@
>      '/db/sqlite3/src',
>      '/dom/base',
> +    '/dom/base',

Remove this line.

::: dom/ipc/moz.build
@@ +101,3 @@
>      '/docshell/base',
>      '/dom/base',
> +    '/dom/base',

Remove this line.

::: dom/notification/moz.build
@@ +30,5 @@
>  include('/ipc/chromium/chromium-config.mozbuild')
>  
>  FINAL_LIBRARY = 'xul'
>  LOCAL_INCLUDES += [
> +    '/dom/base',

Remove this line.

::: dom/offline/moz.build
@@ +13,5 @@
>  
>  FAIL_ON_WARNINGS = True
>  
>  LOCAL_INCLUDES += [
> +    "/dom/base",

Remove this line.

::: dom/plugins/base/moz.build
@@ +95,5 @@
>  
>  MSVC_ENABLE_PGO = True
>  
>  LOCAL_INCLUDES += [
> +    '/dom/base',

Remove this line.

::: layout/base/moz.build
@@ +114,5 @@
>  
>  include('/ipc/chromium/chromium-config.mozbuild')
>  
>  LOCAL_INCLUDES += [
> +    '../../dom/base',

Remove the ../.. like you did elsewhere.

::: uriloader/exthandler/moz.build
@@ +120,5 @@
>  
>  FINAL_LIBRARY = 'xul'
>  
>  LOCAL_INCLUDES += [
> +    '/dom/base',

Remove this line.
Attachment #8507944 - Flags: review?(peterv) → review+
(Assignee)

Comment 107

3 years ago
(In reply to Peter Van der Beken [:peterv] from comment #106)
> @@ -189,5 @@
> >  if CONFIG['MOZ_BUILD_APP'] in ['browser', 'mobile/android', 'xulrunner']:
> >      DEFINES['HAVE_SIDEBAR'] = True
> >  
> > -MOCHITEST_MANIFESTS += ['test/mochitest.ini']
> > -MOCHITEST_CHROME_MANIFESTS += ['test/chrome.ini']
> 
> I don't understand this removal, why is this ok?

Those manifests are referenced in dom/base/test/moz.build, which was moved from content/base/test/moz.build as-is. content/base/test already contained chrome.ini/mochitest.ini, which have now been merged with the existing manifests in dom/base/test.

(In reply to Peter Van der Beken [:peterv] from comment #105)
> What happened to this test?

Got lost on the way, good catch!

Thanks for the quick reviews. I will address the comments and land this on Saturday in order to minimize disturbance.
(Assignee)

Comment 108

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c053db10004f
https://hg.mozilla.org/integration/mozilla-inbound/rev/d245e31f8edc
https://hg.mozilla.org/integration/mozilla-inbound/rev/277005c35f05
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ccc36e1d05c
https://hg.mozilla.org/integration/mozilla-inbound/rev/2db29c0ae60b
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a84cae5edba

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e8392b1f0dbe

Sheriffs, please ping/needinfo? me before backing this out. I made about 40 try pushes and tried to fix all reordering issues I found. If a test is particularly orange on some platform, please disable it on that platform and assign the bug to me rather than backing this out.
https://hg.mozilla.org/mozilla-central/rev/c053db10004f
https://hg.mozilla.org/mozilla-central/rev/d245e31f8edc
https://hg.mozilla.org/mozilla-central/rev/277005c35f05
https://hg.mozilla.org/mozilla-central/rev/8ccc36e1d05c
https://hg.mozilla.org/mozilla-central/rev/2db29c0ae60b
https://hg.mozilla.org/mozilla-central/rev/7a84cae5edba
Depends on: 1089410
Depends on: 1089453
Depends on: 1089456

Updated

3 years ago
Blocks: 1089681

Updated

3 years ago
Depends on: 1089741
Depends on: 1103651
(Assignee)

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.