Closed Bug 442972 Opened 12 years ago Closed 12 years ago

improve support for DOM manipulation handling in native menu system, add tests

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jaas, Assigned: jaas)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch fix v1.0 (obsolete) — Splinter Review
This patch fixes a whole bunch of bugs related to the native menu system's response to DOM manipulations. Cumulative content append notifications work correctly with this patch, as does inserting and removing menus from top-level menu bars. Also fixes multiple bugs in our native menu testing framework, adds a DOM and native state syncing mechanism, and expands tests to include coverage for basic DOM manipulations. Also includes some code cleanup and minor optimizations.
Attachment #327625 - Flags: review?(nick.kreeger)
Comment on attachment 327625 [details] [diff] [review]
fix v1.0

r=me
Attachment #327625 - Flags: review?(nick.kreeger) → review+
Attachment #327625 - Flags: superreview?(roc)
Use PRBool instead of "bool".

+    nsMenuX* newMenu = new nsMenuX();
+    if (newMenu) {
+      nsresult rv = newMenu->Create(this, this, aChild);
+      if (NS_SUCCEEDED(rv))
+        InsertMenuAtIndex(newMenu, aIndexInContainer);
+      else
+        delete newMenu;

Use nsAutoPtr<nsMenuX> and .forget() when necessary to avoid the manual delete.

Would it be a good idea to allow ForceNativeMenuReload to selectively reconstruct particular submenus or whatever actually happens when the user clicks on a menu title in the menubar? Always doing a full reconstruction might mask some bugs.
Comment on attachment 327625 [details] [diff] [review]
fix v1.0

We discussed this on IRC and I suggested we make forceNativeMenuReload more flexible so we can more effectively test the lazy menu reconstruction, before we fix all the bugs.
Attachment #327625 - Flags: superreview?(roc)
Attached patch fix v1.1 (obsolete) — Splinter Review
updated to current trunk, no changes
Attachment #327625 - Attachment is obsolete: true
landed on trunk, tests with lazy updating taken into account are bug 446352
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 447042
I backed this out to fix bug 447042. See changeset ced2734e6a61.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch fix v1.2 (obsolete) — Splinter Review
Fixes the crash in DOM inspector, there was a mismatch concerning assumptions about array indexes. Not ready to go yet though, still looking into some things.
Attachment #330279 - Attachment is obsolete: true
Attached patch fix v1.3 (obsolete) — Splinter Review
Fixes another bug my originally checked-in patch introduced, a bug that prevented a sheet's parent window's menu bar from painting when a sheet closed.
Attachment #331332 - Attachment is obsolete: true
Comment on attachment 331350 [details] [diff] [review]
fix v1.3

r=me
Attachment #331350 - Flags: review+
Attached patch fix v1.4Splinter Review
Contains additional tests for bug 447042.
Attachment #331350 - Attachment is obsolete: true
landed on trunk
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.