Closed Bug 1038570 Opened 5 years ago Closed 5 years ago

Rename nsObjectFrame to nsPluginFrame

Categories

(Core :: Layout, defect)

defect
Not set

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)

It is confusing that <object> doesn't always produce nsObjectFrame but only plugin <object>s.

Rename nsObjectFrame to nsPluginFrame to reflect this.
Mentor: kchen
Whiteboard: [good first bug][lang=c++]
Pedro: take a crack at this one?
Assignee: nobody → peterpadillajr
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)
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)
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)
Attachment #8477932 - Flags: review?(kchen)
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)
Attached patch Patch (v1) (obsolete) — Splinter Review
Attachment #8477952 - Flags: feedback?(kchen)
Attached patch Complete Patch (obsolete) — Splinter Review
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)
Attached patch Patch (v1) (obsolete) — Splinter Review
This invalidates sawrubh's patch. He had uploaded my patch by mistake.
Attachment #8477956 - Flags: feedback?(kchen)
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.
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.
Attachment #8477952 - Flags: feedback?(kchen)
Attachment #8477954 - Flags: review?(kchen)
Attachment #8477956 - Flags: feedback?(kchen)
This patch only moves the files with 'hg mv'
Attachment #8483438 - Flags: review?(kchen)
This patch renames classes and variables in C++ files (.h .hpp .cpp)
Attachment #8483439 - Flags: review?(kchen)
This patch renames variables in non C++ files (.mm anf moz.build)
Attachment #8483440 - Flags: review?(kchen)
Tbpl is green with thoses 3 stacked patch applied:
https://tbpl.mozilla.org/?tree=Try&rev=c86c71441332
Assignee: peterpadillajr → six.dsn
Attachment #8483438 - Flags: review?(kchen) → review?(dbaron)
Attachment #8483439 - Flags: review?(kchen) → review?(dbaron)
Attachment #8483440 - Flags: review?(kchen) → review?(dbaron)
Status: NEW → ASSIGNED
Flags: needinfo?(peterpadillajr)
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+
Attachment #8483439 - Flags: review?(dbaron) → review+
Attachment #8483440 - Flags: review?(dbaron) → review+
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+
Keeping previous dbaron r+
Attachment #8483439 - Attachment is obsolete: true
Attachment #8494504 - Flags: review+
Keeping previous dbaron r+
Attachment #8483440 - Attachment is obsolete: true
Attachment #8494505 - Flags: review+
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)
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)
You need to log in before you can comment on or make changes to this bug.