Last Comment Bug 627956 - Fold widget/src/build into widget/src/windows
: Fold widget/src/build into widget/src/windows
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
Depends on: 679226
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-21 17:52 PST by Zack Weinberg (:zwol)
Modified: 2012-01-04 13:04 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1. (9.81 KB, patch)
2011-12-30 13:04 PST, Brian R. Bondy [:bbondy]
no flags Details | Diff | Review
Patch v2. (7.09 KB, patch)
2012-01-02 12:29 PST, Brian R. Bondy [:bbondy]
no flags Details | Diff | Review
Patch v3. Fixed makefile (9.59 KB, patch)
2012-01-03 12:03 PST, Brian R. Bondy [:bbondy]
roc: review+
Details | Diff | Review

Description Zack Weinberg (:zwol) 2011-01-21 17:52:29 PST
The contents of widget/src/build belong in widget/src/windows.  Whoever does this, please also rename nsWinWidgetFactory.cpp to nsWidgetFactory.cpp for consistency with the other widget impls.

(I would do this myself but build-testing Windows is presently a pain, and it's post-FF4 material and I probably won't remember by the time it comes around again.)
Comment 1 Brian R. Bondy [:bbondy] 2011-08-14 08:21:33 PDT
Jim is this something you would like?
Comment 2 Jim Mathies [:jimm] 2011-08-15 06:10:29 PDT
Not sure if nsWinWidgetFactory.cpp is currently in use by other platforms, we should investigate. Also, we may be able to blow away the the rc file and the cursor files in res.. I'm not sure what those are for. Worth investigating, but not a high priority. cc'ing Roc as he might know more. These resources look ancient.
Comment 3 Zack Weinberg (:zwol) 2011-08-15 08:08:12 PDT
widget/src/Makefile.in adds 'build' to DIRS if and only if it adds 'windows' to DIRS.  So unless something reaches into widget/src from elsewhere in the tree, I'm confident the directories can be merged.  I would recommend Just Doing It and worrying about possibly-obsolete files in a separate bug.
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-15 14:44:38 PDT
Yeah, we should do this.
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-15 14:45:45 PDT
Another thing we should do is move everything in 'widget/src' into 'widget'. Dunno if there's a bug for that.
Comment 6 Jim Mathies [:jimm] 2011-08-15 15:21:23 PDT
Isn't there an issue with our current version of hg where moving files causes a loss of history?
Comment 7 Zack Weinberg (:zwol) 2011-08-15 15:42:33 PDT
(In reply to Jim Mathies [:jimm] from comment #6)
> Isn't there an issue with our current version of hg where moving files
> causes a loss of history?

Yes, see http://mercurial.selenic.com/bts/issue1576 . However, my understanding is that this is not a permanent loss of information, only a failure of information-displaying commands (notably 'annotate') to understand the rename record.  Therefore, I do not think we should delay source tree reorganization because of it.
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-15 15:55:32 PDT
Zack's mostly right; no actual history is lost, but some Mercurial UI doesn't handle file moves well. "annotate" is actually fine, but "log" cuts off (but provides a link to the previous location). Matt Mackall thinks this is desirable behavior and wontfixed the bug :-(. The current state is not too bad and as good as it's going to get, so we may as well go ahead with file moves.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-15 15:56:53 PDT
(Actually we should work around Mackall by patching our hgweb install, but this is all discussion for elsewhere.)
Comment 10 Brian R. Bondy [:bbondy] 2011-12-30 13:04:18 PST
Created attachment 585028 [details] [diff] [review]
Patch v1.

- Removed unused source files and resources
- Renamed nsWinWidgetFactory.cpp to nsWidgetFactory.cpp
- Moved widget/build/nsWidgetFactory.cpp to widget/windows

Will request a review once the try results come in.
Comment 11 Brian R. Bondy [:bbondy] 2012-01-02 12:29:51 PST
Created attachment 585323 [details] [diff] [review]
Patch v2.

Regarding the .rc file and resource files that were removed, I found that the build failed because toolkit\library\xulrunner.rc(1) tries to #include "widget.rc".

I'm not sure if that's important or not but I re-added the files in the new location instead of removing.  If we want to remove them let's do that in a different bug.

Re-pushed to try, will request a review once it succeeds.
Comment 12 Brian R. Bondy [:bbondy] 2012-01-03 12:00:51 PST
try run looks good:
https://tbpl.mozilla.org/?tree=Try&rev=c56aeade4b48
Comment 13 Brian R. Bondy [:bbondy] 2012-01-03 12:03:24 PST
Created attachment 585502 [details] [diff] [review]
Patch v3. Fixed makefile
Comment 14 Brian R. Bondy [:bbondy] 2012-01-03 19:57:47 PST
Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d26fad81f51a
Comment 15 Marco Bonardo [::mak] 2012-01-04 04:56:38 PST
https://hg.mozilla.org/mozilla-central/rev/d26fad81f51a
Comment 16 neil@parkwaycc.co.uk 2012-01-04 13:04:23 PST
Comment on attachment 585502 [details] [diff] [review]
Patch v3. Fixed makefile

> ifneq (,$(filter os2 cocoa qt android gonk,$(MOZ_WIDGET_TOOLKIT)))
> DIRS += $(MOZ_WIDGET_TOOLKIT)
> endif
> 
> ifeq ($(MOZ_WIDGET_TOOLKIT),windows)
>-DIRS += windows build
>+DIRS += windows
> endif
Could have written this:
ifneq (,$(filter windows os2 cocoa qt android gonk,$(MOZ_WIDGET_TOOLKIT)))
DIRS += $(MOZ_WIDGET_TOOLKIT)
endif

Note You need to log in before you can comment on or make changes to this bug.