Closed Bug 1309049 Opened 3 years ago Closed 3 years ago

move embedding/ files out of that directory

Categories

(Core Graveyard :: Embedding: APIs, defect)

defect
Not set

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: myk, Assigned: myk)

References

Details

Attachments

(2 files, 8 obsolete files)

bz: this is a strawman proposal for moving embedding/ files out of that directory. Roughly, it makes these changes:

embedding/components/
    -> docshell/components/

most of embedding/browser/
    -> docshell/components/browser/

embedding/browser/nsDocShellTreeOwner.*|ns*ContextMenu*|nsI*Tooltip*
    -> docshell/base/

embedding/nsIWindow* (and tests)
    -> docshell/components/windowcreator/

embedding/nsEmbedCID.h
    -> docshell/components/browser/

I chose docshell/ because some of the moved files (like nsDocShellTreeOwner) are related, and also because you're its module owner, which provides continuity of ownership over the files.

Nevertheless, I know that docshell/ isn't a great fit for most of the files, ontologically, and I'm happy to move them elsewhere.

Offhand, xpfe/ and toolkit/ seem like better fits, but the former seems moribund, and you aren't a peer of either.
Hmm, Bugzilla lost my attachment, presumably because I tried to ask bz for review, which he isn't accepting at the moment.  Here's the attachment in question.

bsmedberg: perhaps you can give this a quick "feedback" look in the meantime?  For context, here's how I described the changes to bz when originally filing the bug:

bz: this is a strawman proposal for moving embedding/ files out of that directory. Roughly, it makes these changes:

embedding/components/
    -> docshell/components/

most of embedding/browser/
    -> docshell/components/browser/

embedding/browser/nsDocShellTreeOwner.*|ns*ContextMenu*|nsI*Tooltip*
    -> docshell/base/

embedding/nsIWindow* (and tests)
    -> docshell/components/windowcreator/

embedding/nsEmbedCID.h
    -> docshell/components/browser/

I chose docshell/ because some of the moved files (like nsDocShellTreeOwner) are related, and also because you're its module owner, which provides continuity of ownership over the files.

Nevertheless, I know that docshell/ isn't a great fit for most of the files, ontologically, and I'm happy to move them elsewhere.

Offhand, xpfe/ and toolkit/ seem like better fits, but the former seems moribund, and you aren't a peer of either.
Attachment #8799556 - Flags: feedback?(benjamin)
Duplicate of this bug: 1309048
Comment on attachment 8799556 [details] [diff] [review]
move embedding/ files out of that directory

I don't have a lot of feedback, but here's what I do have:

rename to docshell/components/appstartup/nsAppStartupNotifier.h
rename to docshell/components/appstartup/nsIAppStartupNotifier.h

These should move to toolkit/xre (no subdirectory) instead of docshell.

I'm skeptical that docshell is the right place for webbrowserpersist (I tend to think it's more DOM), but that's something bz and the DOM team should decide.
Attachment #8799556 - Flags: feedback?(benjamin)
I don't really have strong views about any of this stuff.  I have no idea where most of this should live at all.  :(
Attachment #8799557 - Attachment is obsolete: true
Attachment #8799558 - Attachment is obsolete: true
Attachment #8799559 - Attachment is obsolete: true
Attachment #8799560 - Attachment is obsolete: true
Attachment #8799561 - Attachment is obsolete: true
(In reply to Benjamin Smedberg [:bsmedberg] from comment #8)
> I don't have a lot of feedback, but here's what I do have:
> 
> rename to docshell/components/appstartup/nsAppStartupNotifier.h
> rename to docshell/components/appstartup/nsIAppStartupNotifier.h
> 
> These should move to toolkit/xre (no subdirectory) instead of docshell.

Ok, this updated patch moves them to that directory.


> I'm skeptical that docshell is the right place for webbrowserpersist (I tend
> to think it's more DOM), but that's something bz and the DOM team should
> decide.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #9)
> I don't really have strong views about any of this stuff.  I have no idea
> where most of this should live at all.  :(

Most of dom/ is owned by subdirectory-specific modules, but the Document Object Model module <https://wiki.mozilla.org/Modules/All#Document_Object_Model> owns the rest, and you're a peer of it.

Are there any parts of this patch that you're comfortable moving to dom/? Perhaps webbrowserpersist, as bsmedberg suggested?  Or would you rather I ask the module owners (jst and peterv)?


mossop: as the owner of Toolkit, and a peer for Browser, are there any other files in embedding/ that you'd move to toolkit/ or browser/?

In case it helps, here's a GitHub view of the patch:

https://github.com/mykmelez/gecko/compare/central...move-embedding-files
Attachment #8799556 - Attachment is obsolete: true
Attachment #8802721 - Flags: feedback?(dtownsend)
Comment on attachment 8802721 [details] [diff] [review]
move embedding/ files out of that directory

My only strong opinion is that I don't think any of it belongs in browser. As for toolkit I could see a case being made for a bunch of it, most of the stuff in embedding/components in fact (webbrowserpersist and commandhandler is more DOM I think).
Attachment #8802721 - Flags: feedback?(dtownsend)
(In reply to Dave Townsend [:mossop] from comment #11)
> Comment on attachment 8802721 [details] [diff] [review]
> move embedding/ files out of that directory
> 
> My only strong opinion is that I don't think any of it belongs in browser.
> As for toolkit I could see a case being made for a bunch of it, most of the
> stuff in embedding/components in fact (webbrowserpersist and commandhandler
> is more DOM I think).

Ok, I've moved the browser, find, printingui, windowcreator, and windowwatcher components to toolkit/; while moving webbrowserpersist and commandhandler to dom/; and putting nsEmbeddingModule.cpp (from embedding/components/build/) into toolkit/xre/.

So the patch no longer puts any files into docshell/components/ (although it still moves some files from embedding/browser/ to docshell/base/).

The complete list of changes is now:

embedding/browser -> toolkit/components/browser

embedding/browser/nsCTooltipTextProvider.h
embedding/browser/nsContextMenuInfo.cpp
embedding/browser/nsContextMenuInfo.h
embedding/browser/nsDocShellTreeOwner.cpp
embedding/browser/nsDocShellTreeOwner.h
embedding/browser/nsIContextMenuListener.idl
embedding/browser/nsIContextMenuListener2.idl
embedding/browser/nsITooltipListener.idl
embedding/browser/nsITooltipTextProvider.idl
  -> docshell/base/

embedding/components/appstartup/nsAppStartupNotifier.cpp
embedding/components/appstartup/nsAppStartupNotifier.h
embedding/components/appstartup/nsIAppStartupNotifier.h
embedding/components/build/nsEmbeddingModule.cpp
  -> toolkit/xre/

embedding/components/commandhandler -> dom/commandhandler

embedding/components/find -> toolkit/components/find

embedding/components/printingui -> toolkit/components/printingui

embedding/components/webbrowserpersist -> dom/webbrowserpersist

embedding/components/windowwatcher -> toolkit/components/windowwatcher

embedding/nsEmbedCID.h
  -> toolkit/components/browser/

embedding/nsIWindowCreator.idl
embedding/nsIWindowCreator2.idl
embedding/nsIWindowProvider.idl
embedding/test
  -> toolkit/components/windowcreator/

Requesting review from mossop for the toolkit/ changes and jst (as bz is unavailable) for the docshell/ and dom/ changes, and to confirm moving some of the files to toolkit/.

Note: this patch was generated with Git's --no-renames option so that Bugzilla's diff/review tools can handle it.  Thus it includes a bunch of new/deleted files.  For a better view of the changes, see:

https://github.com/mykmelez/gecko/compare/central...move-embedding-files
https://github.com/mykmelez/gecko/pull/1
Attachment #8802721 - Attachment is obsolete: true
Attachment #8804492 - Flags: review?(jst)
Attachment #8804492 - Flags: review?(dtownsend)
Attachment #8804492 - Flags: review?(dtownsend) → review+
Comment on attachment 8804492 [details] [diff] [review]
move embedding/ files out of that directory

Very belated r=jst even though this is marked as obsolete. It seems from the diff that this is a bunch of removals+additions instead of file moves. If that's just an artifact of the how the patch was created, then all good, if not, let's make these real file moves before something like this lands.
Attachment #8804492 - Flags: review?(jst) → review+
(In reply to Johnny Stenback  (:jst, jst@mozilla.com) from comment #13)
> Comment on attachment 8804492 [details] [diff] [review]
> move embedding/ files out of that directory
> 
> Very belated r=jst even though this is marked as obsolete.

Thanks for the review!  Here's an updated patch that applies to the current tip.

> It seems from the
> diff that this is a bunch of removals+additions instead of file moves. If
> that's just an artifact of the how the patch was created, then all good, if
> not, let's make these real file moves before something like this lands.

I think it was actually an artifact of the configuration of the repository I used to create it.  I've updated that configuration and recreated the changeset, so the patch now shows these changes as a bunch of file moves (along with the minimal code changes to support them).

To ensure that the changes show up as file moves when pushed upstream (and also to confirm that they don't regress anything), I also pushed them to tryserver, which shows them as file moves:

https://hg.mozilla.org/try/rev/3bf13d048d50c5de6623b875e9f161f1d181e17f

I'll push this inbound once the tryserver run completes successfully:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa4f9e47ab96d7d1e64599d1fa3d0a9986ff8373
Excellent, SGTM!
Blocks: 1330058
eslint failed on try because of issues in toolkit/components/windowcreator/test/ and toolkit/components/windowwatcher/test/.  I'll push this with those directories excluded from linting, then fix those issues in bug 1330058, unless anyone particularly wants me to include those changes with this bug.
The tryserver run subsequently failed at the packaging stage because I hadn't updated the browser/ package manifest to account for the changes to the XPIDL module names (embed_base renamed to windowcreator, find merged into mozfind).

This patch updates that manifest (along with those for b2g/ and mobile/android/), via this obvious fix:

https://github.com/mykmelez/gecko/commit/50535386ef738adc510c44eff4131339493cb67c

Here's a new tryserver run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f36fe04628ecf21f362f4ff9242ba9bd78689c98
Attachment #8825479 - Attachment is obsolete: true
Blocks: 1330757
Pushed by myk@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58c050e010a5
move embedding/ files out of that directory, r=jst,mossop
Blocks: 1330817
Blocks: 1330818
https://hg.mozilla.org/mozilla-central/rev/58c050e010a5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.