Closed Bug 946065 Opened 10 years ago Closed 9 years ago

Move content/ to dom/

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: poiru)

References

(Blocks 1 open bug)

Details

Attachments

(20 files, 3 obsolete files)

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
mozilla
: 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.akhgari
: 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)
Note that moving everything to dom/ means merging content/base with dom/base and content/media with dom/media. That seems OK to me.
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)
(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?
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)
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?
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-
Attachment #8351689 - Flags: review?(Ms2ger) → review+
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) → superreview+
Attachment #8351689 - Flags: superreview?(khuey) → superreview+
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)
Attachment #8357936 - Flags: review?(armenzg) → review+
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+
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?
The events patch makes test_object_plugin_nav.html fail on Mac. https://hg.mozilla.org/try/rev/7cf5e598d868
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)
Attachment #8359557 - Flags: review?(bugs) → review+
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?
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
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)
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 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! :)
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)
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+
Keywords: checkin-needed
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [leave open] → [leave open][checkin parts 6 & 7]
backed out this changes since they might have caused Bug 963244
Depends on: 963244
Attachment #8402395 - Flags: superreview?(jwatt) → review+
Keywords: leave-open
(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)
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)
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+
Really every future patch in this bug should land with a CLOBBER change!
Depends on: 1048349
Attachment #8466710 - Flags: review?(Jan.Varga) → review+
Followup touching clobber, since this will likely need one:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d99f8aaeb32b
(try builds are !clobber)
Depends on: 1084979
Depends on: 1085208
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)
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+
(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.
Depends on: 1089410
Depends on: 1089453
Depends on: 1089456
Blocks: 1089681
Depends on: 1089741
Depends on: 1103651
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.