Bugzilla – Full Text Bug Listing
| Summary: | Non-breaking API change required to support custom property editors in WPE | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [WebTools] Java Server Faces | Reporter: | Cameron Bateman <cameron.bateman> | ||||||||
| Component: | Core | Assignee: | Cameron Bateman <cameron.bateman> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||
| Severity: | blocker | ||||||||||
| Priority: | P1 | CC: | deboer, gerry.kessler, neil.hauge, raghunathan.srinivasan | ||||||||
| Version: | 3.0.4 | Flags: | raghunathan.srinivasan:
pmc_approved?
(david_williams) raghunathan.srinivasan: pmc_approved? (naci.dai) deboer: pmc_approved+ neil.hauge: pmc_approved+ raghunathan.srinivasan: pmc_approved? (kaloyan) raghunathan.srinivasan: review+ |
||||||||
| Target Milestone: | 3.0.4 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Windows XP | ||||||||||
| Whiteboard: | PMC_approved | ||||||||||
| Attachments: |
|
||||||||||
Created attachment 124466 [details]
Fixes for Gerry's code review
Created attachment 124483 [details]
Added copyright headers
I've reviewed the patch, and it looks good. * Explain why you believe this is a stop-ship defect. Or, if it is a "hotbug" (requested by an adopter) please document it as such. This is a "hotbug" request from Oracle. The fix is required to provide enhanced property editor for the Web Page Editor feature in the adopter product. * Is there a work-around? If so, why do you believe the work-around is insufficient? No workaround. * How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added? Existing junits cover the change. The fix has also been verified in the adopter product. * Give a brief technical overview. Who has reviewed this fix? See comment 0 . Gerry Kessler has reviewed the fix. See comment 3 * What is the risk associated with this fix? Low-risk. +1 Out of curiosity, why have the IPropertySheetPageFactory at all? I thought a "lesson learned" from platform was to have the extension point use the abstract class directly with no interface. This is one less interface to maintain, and potential changes in the future are less likely to cause breakage if there is no chance of adopters incorrectly extending the interface. > I thought a "lesson learned" from platform was to have the extension point
> use the abstract
> class directly with no interface. This is one less interface to maintain, and
> potential changes in the future are less likely to cause breakage if there is
> no chance of adopters incorrectly extending the interface.
My rationale for doing it this way is that we have more flexibility in the future if we need to implement an internal extension that is not dependent on constrained API code.
Could you point me to the platform doc on this? What you suggest makes sense and I'm happy to get educated on best practice here.
Not quite sure what you mean about an internal extension since both IPropertySheetPageFactory and AbstractPropertySheetPageFactory are public (?). Sorry, not aware of any docs on this. My general rule: interfaces for things you'll be hiding the implementing internally (like IProject), and abstract classes for things other teams will implement. Just learned from many hours with Jim des Rivieres (former Eclipse API doctor) a few years ago, and learning about cases like IPerspectiveListener where new extension interfaces kept having to be created to avoid breaking API. See IPerspectiveListener4 as an example. :) Just to confirm, I've approved the bug either way. Marking it as PMC approved. Patch applied to 3.0.4. Will mark fixed once 3.1 is synced. Change forward ported to 3.1 stream. |
Created attachment 124454 [details] Non-breaking API addition The attached patch adds a new extension point and a small, low-risk modification to internal code, which allows adopters to replace the property page that is provided for the Web Page Editor.