Closed Bug 730271 Opened 9 years ago Closed 9 years ago

Create sync folder for Sync-related content files

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 13

People

(Reporter: dao, Assigned: dao)

Details

Attachments

(1 file)

Attached patch patchSplinter Review
It's getting messy in browser/base/content...
Attachment #600371 - Flags: review?(mak77)
Comment on attachment 600371 [details] [diff] [review]
patch

Yes, this is probably a good idea, I didn't figure out there were so many sync content files there :/
Though, I think we first need feedback from the Sync team on the change, than I will be more than happy to review the patch.
Attachment #600371 - Flags: feedback?(rnewman)
Comment on attachment 600371 [details] [diff] [review]
patch

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

Fine by me! But let's get signoff from Philipp, first.
Attachment #600371 - Flags: feedback?(rnewman)
Attachment #600371 - Flags: feedback?(philipp)
Attachment #600371 - Flags: feedback+
Attachment #600371 - Flags: feedback?(philipp) → feedback+
Comment on attachment 600371 [details] [diff] [review]
patch

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

I'm starting having some doubts regarding the difference we are delineating between stuff in browser/components/something/content and browser/base/something/.
Do we have a rule of thumb regarding that?
Like, for example, how does this stuff differ from browser/components/certerror? wouldn't that also help by splitting out tests and making them easier to run? (browser/base/tests/ is actually a catch-all folder).
Imo abouthome, newtab, sync, pageinfo should all be moved to browser/components and have "content,tests" structure.
Btw, proceeding with the review now, the above can easily be done even later.

---

r=me with the following addressed:

syncSetup.xul is also used in comments in these 3 points that should be updated to sync/setup.xul
http://mxr.mozilla.org/mozilla-central/search?string=syncSetup.xul&find=syncCommon.css

as well as syncGenericChange.xul should be updated to sync/genericChange.xul here
http://mxr.mozilla.org/mozilla-central/search?string=syncGenericChange.xul&find=syncCommon.css
Attachment #600371 - Flags: review?(mak77) → review+
In my view a component is something pluggable (e.g. an XPCOM component).
(In reply to Dão Gottwald [:dao] from comment #4)
> In my view a component is something pluggable (e.g. an XPCOM component).

Well, xpcom would be a possible rule, if not that it is already broken (see my certerror example).  New features are going to be pluggable, for rapid release (you can disable newtab).  And breaking this xpcom rule would actually bring more tangible gains than following it (better separation, easier to run tests).
Btw, was mostly pointing out some incoherence in our structure.
https://hg.mozilla.org/mozilla-central/rev/777e58e87f14
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.