Closed
Bug 1038570
Opened 8 years ago
Closed 8 years ago
Rename nsObjectFrame to nsPluginFrame
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: kanru, Assigned: Six, Mentored)
References
(Blocks 1 open bug)
Details
(Whiteboard: [good first bug][lang=c++])
Attachments
(3 files, 8 obsolete files)
6.10 KB,
patch
|
Six
:
review+
|
Details | Diff | Splinter Review |
64.52 KB,
patch
|
Six
:
review+
|
Details | Diff | Splinter Review |
27.29 KB,
patch
|
Six
:
review+
|
Details | Diff | Splinter Review |
It is confusing that <object> doesn't always produce nsObjectFrame but only plugin <object>s. Rename nsObjectFrame to nsPluginFrame to reflect this.
Reporter | ||
Updated•8 years ago
|
Mentor: kchen
Whiteboard: [good first bug][lang=c++]
Comment 1•8 years ago
|
||
This probably should have been done as a followup to bug 1156 https://github.com/mozilla/gecko-dev/commit/7002686d0d89718f9cb5531319ab5da97e68a474 as I suggested in bug 1156 comment 4.
Depends on: 1156
Comment 3•8 years ago
|
||
Can you please let me know if you are planning to work on this bug? If not, i have modified the files and have uploaded the patch.
Attachment #8477909 -
Flags: review?(kchen)
Flags: needinfo?(peterpadillajr)
Comment 4•8 years ago
|
||
The diff to nsObjectFrame.cpp/nsPluginFrame.cpp and .h appears to be missing from your patch. (If you need help with Mercurial, let somebody know in the bug, or ask on #mercurial or #developers on irc.mozilla.org.)
Flags: needinfo?(kartik.rao24)
Comment 5•8 years ago
|
||
I have deleted the nsObjectFrame.h/cpp files and have added the nsPluginFrame.h/cpp files. Even addition/deletion changes should be visible on patch?
Flags: needinfo?(kartik.rao24)
Comment 6•8 years ago
|
||
Attachment #8477932 -
Flags: review?(kchen)
Comment 7•8 years ago
|
||
The addition still isn't visible. You should also record the change as a move in hg (using "hg mv --after", see "hg help mv") so that the history and blame for the file follows it. It would also be better to repost the corrected patch as a single patch rather than as two separate patches.
Flags: needinfo?(kartik.rao24)
Comment 8•8 years ago
|
||
Attachment #8477952 -
Flags: feedback?(kchen)
Comment 9•8 years ago
|
||
Patch with all the changes
Attachment #8477909 -
Attachment is obsolete: true
Attachment #8477932 -
Attachment is obsolete: true
Attachment #8477909 -
Flags: review?(kchen)
Attachment #8477932 -
Flags: review?(kchen)
Attachment #8477954 -
Flags: review?(kchen)
Flags: needinfo?(kartik.rao24)
Comment 10•8 years ago
|
||
This invalidates sawrubh's patch. He had uploaded my patch by mistake.
Attachment #8477956 -
Flags: feedback?(kchen)
Comment 11•8 years ago
|
||
Comment on attachment 8477954 [details] [diff] [review] Complete Patch This patch still doesn't have the rename of the files recorded as a move; it shows here as a removal and an addition, which is wrong, and which will make the patch hard to review and unacceptable to land. That said, otherwise, this patch looks more complete than the other patch, since this patch renames the file and renames a bunch of variables that should be renamed. Though those steps could perhaps have been better done in a series of stacked patches rather than one combined patch. It's not clear to me why three of you decided to work on this on the same day, though.
Reporter | ||
Comment 12•8 years ago
|
||
I'm not sure who wants to take the last stab on this but please use |hg mv| to rename files (comment 7) and make rename files and rename variables two or more stacked patches (comment 11) as advised by :dbaron.
Reporter | ||
Updated•8 years ago
|
Attachment #8477952 -
Flags: feedback?(kchen)
Reporter | ||
Updated•8 years ago
|
Attachment #8477954 -
Flags: review?(kchen)
Reporter | ||
Updated•8 years ago
|
Attachment #8477956 -
Flags: feedback?(kchen)
Assignee | ||
Comment 13•8 years ago
|
||
This patch only moves the files with 'hg mv'
Attachment #8483438 -
Flags: review?(kchen)
Assignee | ||
Comment 14•8 years ago
|
||
This patch renames classes and variables in C++ files (.h .hpp .cpp)
Attachment #8483439 -
Flags: review?(kchen)
Assignee | ||
Comment 15•8 years ago
|
||
This patch renames variables in non C++ files (.mm anf moz.build)
Attachment #8483440 -
Flags: review?(kchen)
Assignee | ||
Comment 16•8 years ago
|
||
Tbpl is green with thoses 3 stacked patch applied: https://tbpl.mozilla.org/?tree=Try&rev=c86c71441332
Assignee | ||
Updated•8 years ago
|
Assignee: peterpadillajr → six.dsn
Reporter | ||
Updated•8 years ago
|
Attachment #8483438 -
Flags: review?(kchen) → review?(dbaron)
Reporter | ||
Updated•8 years ago
|
Attachment #8483439 -
Flags: review?(kchen) → review?(dbaron)
Reporter | ||
Updated•8 years ago
|
Attachment #8483440 -
Flags: review?(kchen) → review?(dbaron)
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Updated•8 years ago
|
Flags: needinfo?(peterpadillajr)
Comment 17•8 years ago
|
||
Comment on attachment 8483438 [details] [diff] [review] nsobject_to_nsplugin_move_files.patch Part 1/3 This isn't a useful set of stacked patches since they don't compile on their own. You should squash them together when landing (but please don't lose the rename data). What I was suggesting was one patch that renames the files, which would also change the moz.build files and #includes elsewhere, and then another patch to rename the class, and perhaps a third patch to rename variables (such as variables called "objectFrame") to match the rest of the renaming. Note that attachment 8477954 [details] [diff] [review] contained that third patch, whereas this one doesn't. r=dbaron anyway, conditional on squashing the patches
Attachment #8483438 -
Flags: review?(dbaron) → review+
Updated•8 years ago
|
Attachment #8483439 -
Flags: review?(dbaron) → review+
Updated•8 years ago
|
Attachment #8483440 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 18•8 years ago
|
||
Keeping previous dbaron r+
Attachment #8477952 -
Attachment is obsolete: true
Attachment #8477954 -
Attachment is obsolete: true
Attachment #8477956 -
Attachment is obsolete: true
Attachment #8483438 -
Attachment is obsolete: true
Attachment #8494503 -
Flags: review+
Assignee | ||
Comment 19•8 years ago
|
||
Keeping previous dbaron r+
Attachment #8483439 -
Attachment is obsolete: true
Attachment #8494504 -
Flags: review+
Assignee | ||
Comment 20•8 years ago
|
||
Keeping previous dbaron r+
Attachment #8483440 -
Attachment is obsolete: true
Attachment #8494505 -
Flags: review+
Assignee | ||
Comment 21•8 years ago
|
||
Thoses patches are taking care of dbaron's last comment. Sorry for the misundertanding, i'm not familiar with stacked patchs but now it should be better. Just let me know if it's what you expect. Thanks,
Flags: needinfo?(dbaron)
Comment 22•8 years ago
|
||
Yes, this looks great. Thanks. (It would have been ok to just land as 1 big patch. This is better, though it's not obvious that it's worth the work to fix it up after having done it once already.)
Flags: needinfo?(dbaron)
Assignee | ||
Comment 23•8 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=a6c3c48590fc TBPL build is green
Keywords: checkin-needed
Comment 24•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/22deb0f2260d https://hg.mozilla.org/integration/mozilla-inbound/rev/1de156cbeddd https://hg.mozilla.org/integration/mozilla-inbound/rev/03ed854c4872
Keywords: checkin-needed
Comment 25•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/22deb0f2260d https://hg.mozilla.org/mozilla-central/rev/1de156cbeddd https://hg.mozilla.org/mozilla-central/rev/03ed854c4872
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•