Closed
Bug 946065
Opened 11 years ago
Closed 9 years ago
Move content/ to dom/
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
People
(Reporter: roc, Assigned: poiru)
References
(Blocks 1 open bug)
Details
Attachments
(20 files, 3 obsolete files)
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
Comment 1•11 years ago
|
||
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.
Reporter | ||
Comment 2•11 years ago
|
||
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.
Reporter | ||
Comment 4•11 years ago
|
||
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.
Comment 5•11 years ago
|
||
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.
Reporter | ||
Comment 6•11 years ago
|
||
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.
Comment 7•11 years ago
|
||
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.
Reporter | ||
Comment 9•11 years ago
|
||
Sure. The proposal is to do everything in comment #0 and also move everything under content/ to dom/.
Comment 11•11 years ago
|
||
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.
Comment 12•11 years ago
|
||
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)
Reporter | ||
Comment 13•11 years ago
|
||
Yes.
Reporter | ||
Comment 14•11 years ago
|
||
Note that moving everything to dom/ means merging content/base with dom/base and content/media with dom/media. That seems OK to me.
Reporter | ||
Comment 15•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=5746442456c9
Attachment #8348451 -
Flags: review?(Ms2ger)
Comment 16•11 years ago
|
||
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?
Comment 17•11 years ago
|
||
jmaher, how do we get the mochitest-gl path updated?
Flags: needinfo?(jmaher)
Reporter | ||
Comment 18•11 years ago
|
||
(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!
Comment 19•11 years ago
|
||
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•11 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)
Comment 21•11 years ago
|
||
when you say other branches, do you mean aurora/beta/release, or just trunk based branches?
Comment 22•11 years ago
|
||
all of the above, plus esr*s and b2g*s, if mochitest-gl is running there.
Comment 23•11 years ago
|
||
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.
Reporter | ||
Comment 24•11 years ago
|
||
(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.
Comment 25•11 years ago
|
||
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•11 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)
Comment 27•11 years ago
|
||
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.
Comment 28•11 years ago
|
||
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 29•11 years ago
|
||
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)
Reporter | ||
Comment 30•11 years ago
|
||
(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?
Reporter | ||
Comment 31•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=305a2ebb2913
Attachment #8351624 -
Flags: review?(Ms2ger)
Reporter | ||
Comment 32•11 years ago
|
||
Attachment #8351625 -
Flags: review?(Ms2ger)
Reporter | ||
Comment 33•11 years ago
|
||
Attachment #8351689 -
Flags: review?(Ms2ger)
Comment 34•11 years ago
|
||
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•11 years ago
|
Attachment #8351689 -
Flags: review?(Ms2ger) → review+
Updated•11 years ago
|
Attachment #8351624 -
Flags: review?(Ms2ger) → review+
Comment 35•11 years ago
|
||
Please do ask a peer to sr before landing, though.
Reporter | ||
Comment 36•10 years ago
|
||
(In reply to :Ms2ger from comment #34) > JAR_MANIFESTS += ['jar.mn'] I don't know what I was thinking...
Reporter | ||
Comment 37•10 years ago
|
||
(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...
Reporter | ||
Updated•10 years ago
|
Attachment #8351624 -
Flags: superreview?(khuey)
Reporter | ||
Updated•10 years ago
|
Attachment #8351689 -
Flags: superreview?(khuey)
Attachment #8351624 -
Flags: superreview?(khuey) → superreview+
Attachment #8351689 -
Flags: superreview?(khuey) → superreview+
Reporter | ||
Comment 38•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/67ff9392766b https://hg.mozilla.org/integration/mozilla-inbound/rev/215f5bc6284d
Whiteboard: [leave open]
Reporter | ||
Updated•10 years ago
|
Attachment #8351624 -
Flags: checkin+
Reporter | ||
Updated•10 years ago
|
Attachment #8351689 -
Flags: checkin+
Reporter | ||
Updated•10 years ago
|
Attachment #8351625 -
Attachment is obsolete: true
Comment 39•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/67ff9392766b https://hg.mozilla.org/mozilla-central/rev/215f5bc6284d
Reporter | ||
Comment 40•10 years ago
|
||
Attachment #8356747 -
Flags: review?(Ms2ger)
Reporter | ||
Comment 41•10 years ago
|
||
Attachment #8356748 -
Flags: review?(Ms2ger)
Comment 42•10 years ago
|
||
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 43•10 years ago
|
||
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+
Reporter | ||
Comment 44•10 years ago
|
||
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
Comment 46•10 years ago
|
||
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)
Comment 47•10 years ago
|
||
Attachment #8357936 -
Flags: review?(armenzg)
Updated•10 years ago
|
Attachment #8357936 -
Flags: review?(armenzg) → review+
Reporter | ||
Comment 48•10 years ago
|
||
Thanks!!
Comment 49•10 years ago
|
||
forgot to copy this to the mochitest directory in tests packaging.
Attachment #8357936 -
Attachment is obsolete: true
Attachment #8358057 -
Flags: review?(armenzg)
Updated•10 years ago
|
Attachment #8358057 -
Flags: review?(armenzg) → review+
Reporter | ||
Comment 50•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/243259fda9ab
Reporter | ||
Updated•10 years ago
|
Attachment #8356747 -
Flags: checkin+
Comment 52•10 years ago
|
||
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).
Reporter | ||
Comment 53•10 years ago
|
||
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 54•10 years ago
|
||
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?
Reporter | ||
Comment 56•10 years ago
|
||
The events patch makes test_object_plugin_nav.html fail on Mac. https://hg.mozilla.org/try/rev/7cf5e598d868
Reporter | ||
Comment 57•10 years ago
|
||
Attachment #8359557 -
Flags: review?(bugs)
Reporter | ||
Comment 58•10 years ago
|
||
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•10 years ago
|
Attachment #8359557 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8359560 -
Flags: review?(bugs) → review+
Comment 59•10 years ago
|
||
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?
Reporter | ||
Comment 60•10 years ago
|
||
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
Reporter | ||
Updated•10 years ago
|
Attachment #8356748 -
Flags: checkin+
Reporter | ||
Updated•10 years ago
|
Attachment #8359557 -
Flags: checkin+
Reporter | ||
Updated•10 years ago
|
Attachment #8359560 -
Flags: checkin+
Comment 61•10 years ago
|
||
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 62•10 years ago
|
||
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+
Comment 63•10 years ago
|
||
Attachment #8360476 -
Flags: review?(bugspam.Callek)
Comment 64•10 years ago
|
||
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+
Comment 65•10 years ago
|
||
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.
Comment 66•10 years ago
|
||
something is in production
Updated•10 years ago
|
Summary: Flatten content subdirectories → Move content/ to dom/
Comment 68•10 years ago
|
||
Can Part 1 land now, with the additional change to gl.json?
Comment 69•10 years ago
|
||
https://tbpl.mozilla.org/php/getParsedLog.php?id=33439212&tree=Mozilla-Inbound seems to still use the path directly.
Flags: needinfo?(jmaher)
Comment 70•10 years ago
|
||
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•10 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+
Comment 72•10 years ago
|
||
yes, this had been landed on all branches.
Comment 73•10 years ago
|
||
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.
Comment 74•10 years ago
|
||
a mozharness patch is in production! :)
Assignee | ||
Comment 75•10 years ago
|
||
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 | ||
Comment 76•10 years ago
|
||
Attachment #8402394 -
Flags: superreview?(karlt)
Assignee | ||
Comment 77•10 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=f9f900dbdae5
Attachment #8402395 -
Flags: superreview?(jwatt)
Updated•10 years ago
|
Attachment #8402394 -
Flags: superreview?(karlt) → review+
Comment 78•10 years ago
|
||
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•10 years ago
|
Keywords: checkin-needed
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [leave open] → [leave open][checkin parts 6 & 7]
Comment 79•10 years ago
|
||
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]
Comment 80•10 years ago
|
||
backed out this changes since they might have caused Bug 963244
Updated•10 years ago
|
Attachment #8402395 -
Flags: superreview?(jwatt) → review+
Assignee | ||
Comment 81•10 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•10 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•10 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•10 years ago
|
||
Attachment #8348451 -
Attachment is obsolete: true
Attachment #8462575 -
Flags: review?(roc)
Reporter | ||
Comment 85•10 years ago
|
||
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•10 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 87•10 years ago
|
||
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 88•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8462575 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 89•10 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
Blocks: 1044293
Comment 91•10 years ago
|
||
Really every future patch in this bug should land with a CLOBBER change!
Comment 92•10 years ago
|
||
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•10 years ago
|
||
Attachment #8466710 -
Flags: review?(Jan.Varga)
Updated•10 years ago
|
Attachment #8466710 -
Flags: review?(Jan.Varga) → review+
Assignee | ||
Comment 94•10 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
Comment 95•10 years ago
|
||
Followup touching clobber, since this will likely need one: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d99f8aaeb32b (try builds are !clobber)
Comment 96•10 years ago
|
||
(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
Comment 97•10 years ago
|
||
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 | ||
Comment 98•10 years ago
|
||
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•10 years ago
|
||
Assignee | ||
Comment 100•10 years ago
|
||
Attachment #8507941 -
Flags: review?(peterv)
Assignee | ||
Comment 101•10 years ago
|
||
Assignee | ||
Comment 102•10 years ago
|
||
Attachment #8507944 -
Flags: review?(peterv)
Assignee | ||
Comment 103•10 years ago
|
||
Comment 104•10 years ago
|
||
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 105•10 years ago
|
||
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 106•10 years ago
|
||
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•10 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•10 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.
Comment 109•10 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 110•6 years ago
|
||
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•